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

Make origin offset immutable #612

Merged
merged 3 commits into from
May 1, 2019
Merged

Make origin offset immutable #612

merged 3 commits into from
May 1, 2019

Conversation

toji
Copy link
Member

@toji toji commented Apr 27, 2019

Fixes #580. Changes originOffset to be immutable and establish that the
way that the originOffset changes is by creating a new reference
space with the getOffsetReferenceSpace() method of the base space.
This should hopefully solve several timing issues related to changing the
originOffset mid-frame.

@toji toji added this to the May 2019 Working Draft milestone Apr 27, 2019
@toji toji requested a review from NellWaliczek April 27, 2019 04:59
@toji
Copy link
Member Author

toji commented Apr 27, 2019

This is the same PR as #589, but due to a dumb branch deletion on my part and a dumber restriction on GitHub's part, that PR is now un-re-openable forevermore. So y'all get this one now instead. 😝

FWIW this addresses the feedback given in the previous PR, primarily moving the offset reference space creation mechanism from a constructor to a method, which definitely feels cleaner. PTAL!

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

👍 on the design. A few minor questions

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@toji
Copy link
Member Author

toji commented Apr 30, 2019

Addressed the feedback about the unnecessary new text, and the other two comments appear to be potential non-issues? Please take another look.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

LGTM

Fixes #580. Changes originOffset to be immutable and establish that the
way that the originOffset changes is by constructing a new reference
space with the base space and an additional offset to apply on top of
it. This should hopefully solve several timing issues related to
changing the originOffset mid-frame.
@toji toji force-pushed the immutable_origin_offset branch from 10a60a0 to 917d088 Compare May 1, 2019 00:13
@toji toji merged commit bcdf56b into master May 1, 2019
@toji toji deleted the immutable_origin_offset branch May 1, 2019 18:25
@AdaRoseCannon AdaRoseCannon added the ed:spec Include in newsletter, spec change label May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ed:spec Include in newsletter, spec change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should origin offset be a new reference space instead?
4 participants