Skip to content

Conversation

BastiaanOlij
Copy link
Contributor

This PR adds documentation for the spatial entities implementation being added here: godotengine/godot#107391

@BastiaanOlij BastiaanOlij added this to the 4.x milestone Jun 11, 2025
@BastiaanOlij BastiaanOlij self-assigned this Jun 11, 2025
@BastiaanOlij BastiaanOlij added enhancement topic:xr Related to XR documentation labels Jun 11, 2025
@AThousandShips AThousandShips added the waiting on PR merge PR's that can't be merged until an engine PR is merged first label Jun 11, 2025
Copy link

@JD-The-65th JD-The-65th left a comment

Choose a reason for hiding this comment

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

I noticed a few grammatical issues and typos so I wanted to make a review addressing some of them. I saw that it was also a commonality in this PR to omit commas after dependent clauses, so I left in 2 cases where I noticed it occurred, but I can go back and mark more where I noticed them.

@AThousandShips AThousandShips removed this from the 4.x milestone Jun 12, 2025
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Will do a pass to improve sentence structure as well later

@BastiaanOlij
Copy link
Contributor Author

Thanks for the feedback everyone, just to let you all know, I'm planning on updating this PR after we're certain about the API on the implementation PR just to minimize having to go back and forth on those changes. Will be updating this PR soonish

@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch 7 times, most recently from a5b0b4a to 4edb00e Compare August 6, 2025 05:56


**
Q : Should we be doing update queries here to get position changes for markers??
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly a reminder to myself, I need to add this in. We now do this in the built-in logic as well.

@BastiaanOlij
Copy link
Contributor Author

I think I covered all the comments, only some outstanding stuff around parameter naming which remains a problem.

Also need to find some time to add update queries to the marker example.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Some suggestions for breaking up the sentences, can look at more cases later

@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch from 4edb00e to e0bca81 Compare August 29, 2025 02:54
@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch from e0bca81 to 73c533b Compare August 29, 2025 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement topic:xr Related to XR documentation waiting on PR merge PR's that can't be merged until an engine PR is merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants