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

Change timing of when updateRenderState changes apply #1111

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

toji
Copy link
Member

@toji toji commented Aug 7, 2020

Fixes #1051. Would love @asajeffrey's input on this!

Apologies that this has taken as long as it has to get a PR up. Doing some GH and meeting minutes archeology, the primary concerns about the algorithm as it currently is written seem to be that Alan needs it to allow for non-blocking communication to the XR hardware and otherwise myself and a couple of others just want to ensure that the timing of when the changes apply is reasonably predictable.

This change moves the apply the pending render state execution to immediately after the frame loop executes, and queues a task to do it. Then it also moves the execution of the frame loop into a task on the same queue, which should implicitly ensure that the render state update must finish before the next frame executes. (I believe that the frame loop execution was effectively taking place in a task before anyway, so this just makes that explicit. Please correct me if I'm wrong!)

This gives us the behavior that any changes will apply some time after the frame finishes executing, but the only guarantee is that they will be finished applying by the time the next frame begins executing. This means the most predictable way to interact with the render state is to inspect and update it within a frame, with the expectation that any changes will take effect in the next frame.

This is technically a backwards-compatibility break, but I have a hard time imagining that any developers are strongly dependent on the current timing.


Preview | Diff

@toji toji requested a review from Manishearth August 7, 2020 17:31
@toji
Copy link
Member Author

toji commented Aug 7, 2020

Preemptively /agenda -ing this.

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

This behavior makes sense to me.

Copy link

@asajeffrey asajeffrey left a comment

Choose a reason for hiding this comment

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

Minor question, otherwise LGTM.

index.bs Outdated
</dl>
1. [=Queue a task=] to perform the following steps:
1. Set |activeState| to |newState|.
1. If |oldLayer| is not equal to |activeState|'s {{XRRenderState/baseLayer}}, or the dimensions of |oldLayer| are different from |baseLayer|, [=update the viewports=] for |session|.

Choose a reason for hiding this comment

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

Might be worth making sure that we've put the right hooks in for the layers spec to make changes? E.g. in this case, the layers spec is going to want to do this when the layers array has changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! In this case it seems like, since the layers attribute is defined in this spec, we can check for differences without doing the same sort of "create an algorithm to be overridden" pattern. Gave it a pass with some fairly broad language around detecting changes, let me know what you think!

Copy link

@asajeffrey asajeffrey left a comment

Choose a reason for hiding this comment

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

LGTM

@toji toji merged commit 2cd6a63 into master Aug 7, 2020
@toji toji deleted the updateRenderState-timing branch August 7, 2020 20:52
@toji
Copy link
Member Author

toji commented Aug 7, 2020

Merging this now, given that Manish and Alan have both signed off on it. I think I'll leave it on the agenda to mention as a "heads up" for other implementers.

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.

Difficulty with multiprocess implementation of updateRenderState
3 participants