Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tighten up spec text around frame timings, add hook for frame updates #897

Merged
merged 7 commits into from
Nov 21, 2019

Conversation

Manishearth
Copy link
Contributor

This makes it easier to do things like in immersive-web/webxr-gamepads-module#22 , where we want to be able to insert an action that occurs on the main thread whenever a frame is about to be processed.

r? @NellWaliczek

index.bs Outdated
@@ -863,7 +863,7 @@ When this method is invoked, the user agent MUST run the following steps:

<div class="algorithm" data-algorithm="run-animation-frames">

When an {{XRSession}} |session| receives updated [=viewer=] state from the [=XRSession/XR device=], it runs an <dfn>XR animation frame</dfn> with a timestamp |now| and an {{XRFrame}} |frame|, which MUST run the following steps regardless of if the [=list of animation frame callbacks=] is empty or not:
When an {{XRSession}} |session| receives updated [=viewer=] state from the [=XRSession/XR device=], it runs an <dfn>XR animation frame</dfn> with a timestamp |now| and an {{XRFrame}} |frame| with [=XRFrame/time=] |now|, which MUST run the following steps regardless of if the [=list of animation frame callbacks=] is empty or not:
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel like the right way to specify this. |now| and |frame| are both inputs to the algorithm, but the new text feels like an assignment happening in the algorithm description.

I'd suggest either:

  • Assigning [=XRFrame/time=] to |now| explicitly in the algorithm or
  • Removing |now| from the algorithm description altogether and using [=XRFrame/time=] in it's place elsewhere in the algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Hm... I think we went to far in the other direction. We still need to have a declaration of the XRFrame somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by that, it's constructed in the algorithm itself (as it should be, IMO, previously it just appeared out of the ether)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, my bad, i pushed up a partial commit because I forgot to save

Copy link
Member

Choose a reason for hiding this comment

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

🤣

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

Thanks for the update! (And then the actual update with all the things. 😉) LGTM modulo one apparent typo. I'd like Nell to weigh in on it prior to merge, though.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated

</div>

NOTE: Further specs may add new kinds of frame updates to these steps, e.g. for gamepad state.
Copy link
Member

Choose a reason for hiding this comment

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

This seems somewhat odd to me, but I'm not sure what else to suggest. Is there someone with experience with this sort of problem that we could get a sanity check from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternate plan was to have a "list of frame updates" (a list of callbacks) on the session and session with gamepads append a frame update to it. I felt that might get a bit unweildy though.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@Manishearth
Copy link
Contributor Author

Updated.

@Manishearth Manishearth merged commit 7cd6fde into immersive-web:master Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants