✨ [amp-story-360] Add scene orientation support (fixes #29514) #30138

t0mg · 2020-09-07T16:41:02Z

First step for fixing #29514. This doesn't add the logic but since the validator has its own release cycle, we can get this out of the way first.

amp-owners-bot · 2020-09-07T16:41:10Z

Hey @gmajoulet, @processprocess! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-360/0.1/test/validator-amp-story-360.html
extensions/amp-story-360/0.1/test/validator-amp-story-360.out
extensions/amp-story-360/validator-amp-story-360.protoascii

t0mg · 2020-09-07T19:33:43Z

Added necessary shader modifications to Zuho library by @nicoptere to this PR to avoid another branch mess. Depending on the speed of the review I might or might not add the changes to the amp-story-360 player as well :)

t0mg · 2020-09-08T13:01:49Z

Ok, hold on as it seems it would make more sense to take Euler angles (heading, pitch, roll) instead of tilt values. We need to come up with different attribute names (and I need to make changes to zuho as well). Will update this thread when the PR is ready for review again.

t0mg · 2020-09-08T19:58:54Z

Ok, PTAL.

The updated API is scene-heading, scene-pitch and scene-roll. We ditched the yaw/tilt-yaw/tilt-pitch and quaternion implementation in favor of new attributes that rely on Euler angles instead. It better matches Zuho implementation and most importantly it feels less quirky / specific to Street View panoramas. The scene-* coordinates are absolute and applied as a rotation matrix to the rendered panorama.

Open question: this is called orientation in Street View world so @nicoptere suggested media-orientation-* instead of scene-*. While it is more accurate it's also quite a bit longer. Any preference @gmajoulet ? Now is a good time to arbitrate :)

Here is an amp-story-360 rendering a Street View panorama captured from a heavily tilted camera:

Adding the new attributes with matching values scene-heading="358.5595092773438" scene-pitch="8.813064575195312" scene-roll="0.1009868159890175", we get the correct orientation in the viewer (to be compared with the native player):

Gregable

Validator changes look good. I didn't review the javascript.

gmajoulet · 2020-09-08T21:09:23Z

cc @newmuis @ampproject/wg-stories (I'm OOO)

newmuis

scene- sounds good to me for the API prefix! It's generic enough.

third_party/zuho/zuho.js

newmuis · 2020-09-09T01:45:54Z

third_party/zuho/zuho.js

-      else {
-        return vec4(0.0);
-      }
+    vec4 sample( float dx, float dy ) {


yeah... I'll be honest: I only understand the whitespace changes in this file 😅

Do we have any kind of inline comments to make this clearer? Or tests to assert the correct behavior?

As is, I have no idea whether this has broken something

hey,
the zuho.js is a lightweight wrapper around a WebGL context, it is meant to display a collection of projections (formarly stored under zuho.Mapping) in the shader the bool unproject(vec2, out vec3); method was meant to be replaced with different implementations corresponding to the different projections ( if( unproject(p, q) ){ in the next diff).
the goal of this unprojection was to convert the position of a 2D pixel into a 3D coordinate on a sphere to know where to sample the panorama texture.
with some projections, this would result in invalid coordinates, which in turn would give invalid texture sampling coordinate.
this condition was here to handle the invalid cases and fallback to a transparent pixel ( }else{ return vec4(0.); } in the next diff)
branching in a shader is usually considered bad practice but more importantly, the projection we use is always valid as you can see from the previous commit L.88
so what we did was "simply" to remove the test (if(unproject(p,q){)else{}) and inline the resulting 3D vector of the unproject method ( vec3 q = vec3(vPos + uPxSize * vec2(dx, dy), -1.0); )
not sure if it's clearer but that's what happened here ^^'

Thanks for the catch @nicoptere ! The only projection method we are using did not require this test and branching is costly, so it was not just dead code, but expensive dead code 😬. Which reminds me I need to start looking at @jonlab's EAC projection implementation and figure out how to remove all the branching.

As for the whitespace changes in the shader, it's meant to save a few bytes in the JS package: because it's a string literal, it does not get obfuscated/minimzed by the JS compiler like the rest of the code. We could easily shred more bytes, but the shader would become even more obscure 😅

So, are these added lines pulling in code that we'd previously trimmed from the zuho library? Or is this net new code?

There is no new logic in the shader with the latest implementation, only a small refactor/simplification. There were some heavier shader changes in the first proposed implementation (see this commit) but now the new logic has moved entirely to the JS side with the orientation property and the new euler_ method.

Note however that more substantial changes will be required in the shader when we add EAC projection mapping for the video support. And I have no idea how we can add tests to a webgl shader.

🤔

Are there ways we can add a manual test at least? Providing steps to test and expected results?

Definitely something to think about when we make these changes. Maybe screenshots would work with Percy?

Sounds good. I can file an issue to follow up; LGTM for now.

t0mg · 2020-09-11T22:27:28Z

Thanks for the review! Can you rerun the tests? Also is the PR blocked on @gmajoulet ?

t0mg · 2020-09-14T13:12:50Z

/to @rsimha for OWNERS approval :)

t0mg · 2020-09-16T12:39:08Z

anyone willing to owners-approve for third-party? @kristoferbaxter maybe? 🙏

Gregable

approval: validator changes only

gmajoulet

cc @rsimha for /third_party OWNERS approval

…) (ampproject#30138) * Base element with a working example * Add amp-img to test file * Fix base test, add play/pause/rewind methods * revert example to proper state * revert example to master state (for now) * actually revert example to master state (with new line) * Add comment in zuho's README.amp * Implement play/pause support for amp-story-360 in amp-story-page * Fix test * Update/add validator tests * restore missing newlines (wat) * more newline nonsense * Add tilt attributes to validator * Update example * Add orientation correction support to zuho library * Replacing tilt API with Euler angles for scene orientation correction * Remove unused Quaternion class * Whitespace & validator text fixes * Lint fix * Lint fix 2 * Lint fix 3 the fixening

t0mg added 13 commits September 7, 2020 16:37

Base element with a working example

5ec5b35

Add amp-img to test file

392693d

Fix base test, add play/pause/rewind methods

c80067a

revert example to proper state

3aa9304

revert example to master state (for now)

8262755

actually revert example to master state (with new line)

a0cbf8e

Add comment in zuho's README.amp

3ffb36c

Implement play/pause support for amp-story-360 in amp-story-page

bac8e49

Fix test

72f1ab0

Update/add validator tests

e9f0271

restore missing newlines (wat)

f540be9

more newline nonsense

1200fd3

Add tilt attributes to validator

2278202

amp-owners-bot bot requested a review from gmajoulet September 7, 2020 16:41

google-cla bot added the cla: yes label Sep 7, 2020

t0mg added 2 commits September 7, 2020 18:44

Update example

eb4bc95

Add orientation correction support to zuho library

fb243e1

t0mg changed the title ~~✅ [amp-story-360] Add tilt-yaw and tilt-pitch attributes to validator~~ ✅ [amp-story-360] Add tilt-yaw and tilt-pitch attributes support Sep 7, 2020

Replacing tilt API with Euler angles for scene orientation correction

f9a8890

Remove unused Quaternion class

9418f4b

Gregable approved these changes Sep 8, 2020

View reviewed changes

newmuis self-requested a review September 9, 2020 01:38

newmuis reviewed Sep 9, 2020

View reviewed changes

Whitespace & validator text fixes

50284dd

t0mg changed the title ~~✅ [amp-story-360] Add tilt-yaw and tilt-pitch attributes support~~ ✨ [amp-story-360] Add scene orientation support (fixes #29514) Sep 9, 2020

t0mg requested a review from newmuis September 10, 2020 16:08

t0mg added 3 commits September 11, 2020 15:14

Lint fix

ab3adcd

Lint fix 2

0633a3f

Lint fix 3 the fixening

d437068

newmuis approved these changes Sep 11, 2020

View reviewed changes

Gregable approved these changes Sep 16, 2020

View reviewed changes

gmajoulet approved these changes Sep 16, 2020

View reviewed changes

gmajoulet requested a review from rsimha September 16, 2020 19:50

rsimha approved these changes Sep 17, 2020

View reviewed changes

gmajoulet merged commit 43d58d9 into ampproject:master Sep 17, 2020

t0mg deleted the horizon branch September 18, 2020 06:26

processprocess mentioned this pull request Sep 18, 2020

[Story 360] Gyroscope default heading #30002

Closed

banaag added a commit to banaag/amphtml that referenced this pull request Sep 22, 2020

cl/332289095 Revision bump for ampproject#30138

9e6c32d

banaag mentioned this pull request Sep 22, 2020

Validator Rollup #30340

Merged

banaag added a commit to banaag/amphtml that referenced this pull request Sep 22, 2020

cl/332289095 Revision bump for ampproject#30138

b2a3c3f

banaag added a commit that referenced this pull request Sep 22, 2020

cl/332289095 Revision bump for #30138 (#30340)

6819c4a

ampprojectbot added the PR use: In Beta / Experimental label Sep 23, 2020

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020

cl/332289095 Revision bump for ampproject#30138 (ampproject#30340)

fd1f2f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [amp-story-360] Add scene orientation support (fixes #29514) #30138

✨ [amp-story-360] Add scene orientation support (fixes #29514) #30138

t0mg commented Sep 7, 2020

amp-owners-bot bot commented Sep 7, 2020 •

edited

t0mg commented Sep 7, 2020

t0mg commented Sep 8, 2020

t0mg commented Sep 8, 2020 •

edited

Gregable left a comment

gmajoulet commented Sep 8, 2020

newmuis left a comment

newmuis Sep 9, 2020

nicoptere Sep 9, 2020 •

edited

t0mg Sep 9, 2020 •

edited

newmuis Sep 10, 2020

t0mg Sep 10, 2020

newmuis Sep 10, 2020

t0mg Sep 11, 2020

newmuis Sep 11, 2020

t0mg commented Sep 11, 2020

t0mg commented Sep 14, 2020

t0mg commented Sep 16, 2020

Gregable left a comment •

edited

gmajoulet left a comment

✨ [amp-story-360] Add scene orientation support (fixes #29514) #30138

✨ [amp-story-360] Add scene orientation support (fixes #29514) #30138

Conversation

t0mg commented Sep 7, 2020

amp-owners-bot bot commented Sep 7, 2020 • edited

t0mg commented Sep 7, 2020

t0mg commented Sep 8, 2020

t0mg commented Sep 8, 2020 • edited

Gregable left a comment

Choose a reason for hiding this comment

gmajoulet commented Sep 8, 2020

newmuis left a comment

Choose a reason for hiding this comment

newmuis Sep 9, 2020

Choose a reason for hiding this comment

nicoptere Sep 9, 2020 • edited

Choose a reason for hiding this comment

t0mg Sep 9, 2020 • edited

Choose a reason for hiding this comment

newmuis Sep 10, 2020

Choose a reason for hiding this comment

t0mg Sep 10, 2020

Choose a reason for hiding this comment

newmuis Sep 10, 2020

Choose a reason for hiding this comment

t0mg Sep 11, 2020

Choose a reason for hiding this comment

newmuis Sep 11, 2020

Choose a reason for hiding this comment

t0mg commented Sep 11, 2020

t0mg commented Sep 14, 2020

t0mg commented Sep 16, 2020

Gregable left a comment • edited

Choose a reason for hiding this comment

gmajoulet left a comment

Choose a reason for hiding this comment

amp-owners-bot bot commented Sep 7, 2020 •

edited

t0mg commented Sep 8, 2020 •

edited

nicoptere Sep 9, 2020 •

edited

t0mg Sep 9, 2020 •

edited

Gregable left a comment •

edited