search

immersive-web / webxr-polyfill

Use the WebXR Device API today, providing fallbacks to native WebVR 1.1 and Cardboard
Apache License 2.0
381 stars 84 forks source link

Fixes to updateRenderState #102

Closed lojjic closed 4 years ago

lojjic commented 4 years ago

Tests fail but they appear to be generally broken so assuming that's ok.

takahirox commented 4 years ago

Nice fix! I'm facing a problem that session.renderState.foo attributes are undefined which can be resolved by this PR. Let me elaborate a bit for reviewers.

In the current implementation there are two problems with render state attributes.

  1. render state attributes are stored in this[PRIVATE].config.foo inside of XRRenderState https://github.com/immersive-web/webxr-polyfill/blob/master/src/api/XRRenderState.js#L32 but its getter methods return this[PRIVATE].foo. https://github.com/immersive-web/webxr-polyfill/blob/master/src/api/XRRenderState.js#L38 Then it returns undefined.

  2. pendingRenderState is created as an object, not XRRenderState instance, and just being copied to activeRenderState in next frame in XRSession.updateRenderState()https://github.com/immersive-web/webxr-polyfill/blob/master/src/api/XRSession.js#L390-L391 so session.renderState won't have getter methods then session.renderState.foo will be undefined.

This PR fixes these two problems.

toji commented 4 years ago

LGTM! Thanks! (And yes, tests need to be overhauled.)