Add documentation about OpenXR spatial entities#11015
Add documentation about OpenXR spatial entities#11015mhilbrunner merged 1 commit intogodotengine:masterfrom
Conversation
AThousandShips
added
the
waiting on pr merge
JD-The-65th
left a comment
JD-The-65th
left a comment
There was a problem hiding this comment.
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
left a comment
AThousandShips
left a comment
There was a problem hiding this comment.
Will do a pass to improve sentence structure as well later
|
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
force-pushed
the
openxr_spatial_entities_docs
branch
7 times, most recently
from
a5b0b4a to
4edb00e
Compare
|
|
||
|
|
||
| ** | ||
| Q : Should we be doing update queries here to get position changes for markers?? |
There was a problem hiding this comment.
Mostly a reminder to myself, I need to add this in. We now do this in the built-in logic as well.
There was a problem hiding this comment.
Is this still something you plan to add? Or, should this be removed at this point?
|
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. |
AThousandShips
left a comment
AThousandShips
left a comment
There was a problem hiding this comment.
Some suggestions for breaking up the sentences, can look at more cases later
BastiaanOlij
force-pushed
the
openxr_spatial_entities_docs
branch
2 times, most recently
from
e0bca81 to
73c533b
Compare
BastiaanOlij
force-pushed
the
openxr_spatial_entities_docs
branch
from
73c533b to
b04e6b1
Compare
Repiteo
removed
the
waiting on pr merge
|
The source PR has since been merged, so this should be prioritized once the style changes are accounted for |
AThousandShips
left a comment
AThousandShips
left a comment
There was a problem hiding this comment.
I believe it looks good now
BastiaanOlij
force-pushed
the
openxr_spatial_entities_docs
branch
3 times, most recently
from
3f59079 to
dcbf8f6
Compare
|
Not entirely sure why its not finding some things in the class definitions because those classes should be registered. Unless I'm overlooking a typo or mistake in the references? |
BastiaanOlij
force-pushed
the
openxr_spatial_entities_docs
branch
2 times, most recently
from
c2523fc to
71ba12c
Compare
dsnopek
left a comment
dsnopek
left a comment
There was a problem hiding this comment.
Thanks, this looks great!
Only have a bunch of minor notes about type-o's, making some bits a little clearer, and what-not
| It has a very modular design. The core of the API defines how real world entities are structured, | ||
| how they are found, and how information about them is stored and accessed. | ||
|
|
||
| Various extensions are added on top that implement specific systems such as marker tracking, |
There was a problem hiding this comment.
| Various extensions are added on top that implement specific systems such as marker tracking, | |
| Various extensions are added on top that to implement specific systems such as marker tracking, |
There was a problem hiding this comment.
that to? Doesn't flow right.. maybe Various extensions are added on top, which implement specific systems such as marker tracking, ?
|
|
||
|
|
||
| ** | ||
| Q : Should we be doing update queries here to get position changes for markers?? |
There was a problem hiding this comment.
Is this still something you plan to add? Or, should this be removed at this point?
|
@dsnopek |
BastiaanOlij
force-pushed
the
openxr_spatial_entities_docs
branch
from
71ba12c to
fb64d21
Compare
|
Not sure what the CI failure is about, doesn't seem related to the PR? |
|
It's a recent regression unrelated to this, caused by a property being removed and still used in the manual |
dsnopek
left a comment
dsnopek
left a comment
There was a problem hiding this comment.
Thanks!
Skimming the latest changes, I think this looks great :-)
If you rebase again, that may fix the CI issue?
BastiaanOlij
force-pushed
the
openxr_spatial_entities_docs
branch
from
fb64d21 to
57d0d8d
Compare
Let's try, would be good to get this merged. |
|
Thank you, and thanks everyone for the reviews! |
8 participants
This PR adds documentation for the spatial entities implementation being added here: godotengine/godot#107391