Skip to content

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

@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

toji added 3 commits April 30, 2019 17:10
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