Skip to content

Make origin offset immutable#612

Merged
toji merged 3 commits into
masterfrom
immutable_origin_offset
May 1, 2019
Merged

Make origin offset immutable#612
toji merged 3 commits into
masterfrom
immutable_origin_offset

Conversation

@toji

@toji toji commented Apr 27, 2019

Copy link
Copy Markdown
Member

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

toji commented Apr 27, 2019

Copy link
Copy Markdown
Member Author

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!

@NellWaliczek NellWaliczek left a comment

Copy link
Copy Markdown
Member

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

Comment thread index.bs Outdated
Comment thread index.bs
Comment thread index.bs Outdated
@toji

toji commented Apr 30, 2019

Copy link
Copy Markdown
Member Author

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

@NellWaliczek NellWaliczek left a comment

Copy link
Copy Markdown
Member

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