Skip to content

Conversation

@skyace65
Copy link
Contributor

@skyace65 skyace65 commented Jan 15, 2025

Primarily updates the setting up XR page for Godot 4.3. Some images are used on other pages as well hence why they're updated here. I plan to go through those other pages in different PRs in the future, focusing on one page for this just to make things easier to review.

Did the following:

  • All images in this section have been updated and gone through squoosh compression
  • Removed the section about enabling XR shaders and merged those instructions with the instructions for enabling OpenXR. A lot of the text was "we have to do things differently compared to Godot 3 now" Which seems unnecessary at this point in Godot 4s life.
  • I've taken the note about which renderer to use and just made it a small section
  • Add text that acknowledges the warning icons that show up when adding XRController3D nodes
  • I've removed the notes about updating both warnings. I don't think these are particularly necessary, things changing in the future is generally expected. And I'm not confident they will lead to this page being more up to date
  • Removed the "As setup is brought forward with OpenXR" part of the text in one paragraph, it doesn't read well. Also if I'm interpreting it correctly what it's referring to has happened, more OpenXR settings are in this section now compared to when Godot 4.0 released

@BastiaanOlij Let me know if there's any of my changes you'd like modified, If there's anything else on this page you want updated. Or if I should pay particular attention to any of the other XR pages for text updates as I go through them.

@skyace65 skyace65 added enhancement topic:xr Related to XR documentation area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.3 labels Jan 15, 2025
@skyace65 skyace65 requested a review from BastiaanOlij January 15, 2025 22:54
@skyace65 skyace65 force-pushed the Setup-XR branch 2 times, most recently from c25335c to 1a2fb5b Compare January 15, 2025 22:56
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I don't have anything to add that @tetrapod00 hasn't already mentioned. With the openxr settings page I do think the links to the class reference are likely overkill. We should probably link the openxr settings page there if we haven't already.

@tetrapod00
Copy link
Contributor

In general, we should always link to the project settings when mentioning them. Much of the time the ProjectSettings class reference is the only place that includes robust descriptions. In the case of OpenXR, since there is a separate page just for the OpenXR settings (with longer descriptions), linking to the section headers in that page instead of the ProjectSettings class reference page seems fine instead.

I do think that the exact name of the setting as it appears in the UI should at least be mentioned, whether or not it links anywhere, so that the page can be read without needing to rely on the image.

@skyace65
Copy link
Contributor Author

I've made tetrapod's suggestions, and added a link to the OpenXR settings manual page.

@skyace65 skyace65 requested a review from tetrapod00 January 16, 2025 23:17
Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks fine to me, if you were waiting for my approval. Feel free to merge

@mhilbrunner mhilbrunner merged commit 2062dff into godotengine:master Jan 18, 2025
1 check passed
@mhilbrunner
Copy link
Member

In this goes then. Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.3 enhancement topic:xr Related to XR documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants