Add gltf sample viewer #1962

UX3D-haertl · 2021-01-25T15:02:26Z

This PR integrates the current alpha version (published to npm) of the gltf-sample-viewer from Khronos into the render fidelity tools.

I did not include the golden screenshots, since on my machine the scrollbar is visible on the right side. This also happens for me with the babylon and filament renderers. There is already an issue for filament: #720

Furthermore, I suppressed a typescript error during importing the npm package (error TS7016: Could not find a declaration file), which seems to appear if someone wants to import js into ts. I did not see how this was solved with the other renderers.

google-cla · 2021-01-25T15:02:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-01-25T15:06:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

elalish · 2021-01-25T17:42:10Z

Thanks, glad to see this! It looks like you may need to run npm run update:package-lock from the root of the repo, and then commit all the files that changes. Also, can you sign the CLA? I can't take your code until you do.

UX3D-haertl · 2021-01-26T08:13:48Z

@googlebot I signed it!

google-cla · 2021-01-26T08:14:04Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

UX3D-haertl · 2021-01-26T08:48:21Z

@googlebot I signed it!

UX3D-haertl · 2021-01-26T08:52:58Z

npm run update:package-lock didn't work for me, since npm i --only=production deletes the content of node_modules/.bin.
If I run it without the production flag the following error occurs:
'@google/model-viewer-shared-assets@^0.0.1' is not in the npm registry.

elalish · 2021-01-26T16:58:59Z

I'm guessing you just did something in the wrong order. You have an updated package-lock file, but it's at the root where you didn't change the package.json. Go ahead and revert that file. Since you've made a change to package.json in the fidelity package, that's the package-lock file that should change when you run npm run update:package-lock. You'll need to commit it.

elalish · 2021-01-26T16:59:50Z

After you've made these changes, test it by running npm run bootstrap at the root, and then npm run build and npm test.

UX3D-haertl · 2021-01-27T08:53:53Z

I reverted all my package.lock.json files but the error ('@google/model-viewer-shared-assets@^0.0.1' is not in the npm registry.) still appears. I tried deleting all node_modules and executing npm run update:package-lock in root and in packages/render-fidelity-tools. In both cases the error appears. '@google/model-viewer-shared-assets@^0.0.1' seems to be a private package. I found this closed issue #1021 but npm run bootstrap runs fine for me. Is it possible that one can only run npm run update:package-lock if one has access to the private npm package?

elalish · 2021-01-28T16:29:56Z

shared-assets is just another sub-package of our repo, so you've already got it. Can you be very specific about what commands you're running in which directories? It sounds like you might be down in a package when you should be at the repo root.

…ampleViewer # Conflicts: # packages/render-fidelity-tools/package-lock.json # packages/render-fidelity-tools/package.json

UX3D-haertl · 2021-01-29T08:41:34Z

I created all goldens yesterday and run the test, and now it seems to work. I'm not exactly sure what changed.
The test here fails since I didn't upload the goldens yet. But as I mentioned above, on my machine a scrollbar appears on the right side of the screenshot (this happens for me with babylon, filament and gltf-sample-viewer). Do you know how I can resolve this?

elalish · 2021-01-29T17:29:09Z

Great, thanks! Now that the unit tests are passing, I'll update the screenshots and push them for you.

elalish · 2021-01-29T23:29:39Z

I don't have access to your fork, so I pushed my own version with the updated goldens: #1990, so you can look there. Did you actually look at the renders you were producing and compare them to the others? Things that need work:

Camera config
It's not rendering with a transparent background
Your renders are dark; possibly needs the same tone mapping adjustment we added for Babylon
DirectionalLightTest hangs, possibly due to not handling both types of HDR files.

UX3D-haertl · 2021-02-01T07:51:44Z

Thanks. Yes the npm package is still in development and should be finished in the next two weeks. I already resolved most of the issues locally, but I have to wait until we update the package. I will comment again when this is finalized. Should I work on your PR or on this one? I can also give you access to our fork

elalish · 2021-02-01T16:52:13Z

No problem. Might be easiest to work on this branch and give me access to your fork.

UX3D-haertl · 2021-02-08T15:18:42Z

I sadly cannot give you access to our fork, since we would need to pay for this. So it would be good if you create a new PR with the goldens when all is updated.
Do you know how to resolve the current CLA message? (CLAs are signed, but unable to verify author consent)

elalish · 2021-02-08T19:32:57Z

Okay, to make this simple how about you go ahead and upload your own goldens when they're ready (even if they have the scrollbars). That will allow us to do the visual inspections. I'll do a separate PR after that's merged to update them and get rid of the scrollbars. Are you familiar with using our results viewer to see the side-by-sides in your local commit?

As for the CLA, the issue is almost always with email address, as that's how the googlebot tracks a given user. Check that all of your commits were authored with the same Git email that you used to sign the CLA. If you don't set user.email then it won't work.

UX3D-haertl · 2021-02-10T16:12:42Z

Yes I'm familiar with the tests/html page. I will upload the goldens tomorrow or on Friday.

For the CLA, I checked all commits so the user.email should not be a problem. Maybe there is a conflict, because we first tried to sign the employer CLA, but that didn't seem to work. Therefore, I also signed the personal one. Can I somehow get a more useful error message? Maybe with the google-cla bot? unable to verify author consent sounds like I need to approve something, but I'm not sure how to do it.

elalish · 2021-02-11T20:33:38Z

@googlebot I signed it!

elalish · 2021-02-11T20:34:13Z

Looks like the CLA is happy now; must have just not refreshed.

# Conflicts: # packages/render-fidelity-tools/package-lock.json # packages/render-fidelity-tools/package.json

UX3D-haertl · 2021-02-12T15:30:13Z

I uploaded the goldens, but there is currently an issue with blending which will be resolved on Monday.
When this issue is resolved, we can merge this PR or is there anything else to do?

elalish

This looks great, thanks!

UX3D-haertl · 2021-02-15T15:27:45Z

I have updated the gltf sample viewer version. I did not upload the new goldens since they need to be redone anyways.

elalish · 2021-02-16T17:20:13Z

@UX3D-haertl Thanks, see the comments on #2018; I need these issues addressed so that our comparisons can be useful. Thanks!

UX3D-haertl added 7 commits January 21, 2021 13:55

WIP add gltf sample viewer

c43481a

Add index.html

838fbee

Further integration (renderer already running)

86abba0

Fix rotations

af1a159

Fix clearcolor and zoom

533528a

Change ACES parameter

7bc1be9

Use gltf-viewer from npm

df60f4a

UX3D-haertl force-pushed the addGltfSampleViewer branch from dae3551 to df60f4a Compare January 27, 2021 09:03

UX3D-haertl added 2 commits January 29, 2021 09:20

Add package-lock

8bba323

Merge branch 'master' of github.com:google/model-viewer into addGltfS…

372303b

…ampleViewer # Conflicts: # packages/render-fidelity-tools/package-lock.json # packages/render-fidelity-tools/package.json

elalish mentioned this pull request Jan 29, 2021

Add gltf sample viewer #1990

Closed

UX3D-haertl added 5 commits February 12, 2021 09:34

Update sample viewer and resolve issues

82a9119

Merge remote-tracking branch 'origin/master' into addGltfSampleViewer

68a68b3

# Conflicts: # packages/render-fidelity-tools/package-lock.json # packages/render-fidelity-tools/package.json

Update package-lock

097755b

Disable environment blur

4ce8b27

Upload goldens

2b74e46

elalish previously approved these changes Feb 12, 2021

View reviewed changes

Update version of sample viewer

301036d

UX3D-haertl dismissed elalish’s stale review via 301036d February 15, 2021 15:25

elalish approved these changes Feb 16, 2021

View reviewed changes

elalish merged commit cee02f7 into google:master Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gltf sample viewer #1962

Add gltf sample viewer #1962

UX3D-haertl commented Jan 25, 2021

google-cla bot commented Jan 25, 2021

google-cla bot commented Jan 25, 2021

elalish commented Jan 25, 2021

UX3D-haertl commented Jan 26, 2021

google-cla bot commented Jan 26, 2021

UX3D-haertl commented Jan 26, 2021

UX3D-haertl commented Jan 26, 2021

elalish commented Jan 26, 2021

elalish commented Jan 26, 2021

UX3D-haertl commented Jan 27, 2021

elalish commented Jan 28, 2021

UX3D-haertl commented Jan 29, 2021

elalish commented Jan 29, 2021

elalish commented Jan 29, 2021

UX3D-haertl commented Feb 1, 2021

elalish commented Feb 1, 2021

UX3D-haertl commented Feb 8, 2021

elalish commented Feb 8, 2021

UX3D-haertl commented Feb 10, 2021

elalish commented Feb 11, 2021

elalish commented Feb 11, 2021

UX3D-haertl commented Feb 12, 2021

elalish left a comment

UX3D-haertl commented Feb 15, 2021

elalish commented Feb 16, 2021

Add gltf sample viewer #1962

Add gltf sample viewer #1962

Conversation

UX3D-haertl commented Jan 25, 2021

google-cla bot commented Jan 25, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Jan 25, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

elalish commented Jan 25, 2021

UX3D-haertl commented Jan 26, 2021

google-cla bot commented Jan 26, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

UX3D-haertl commented Jan 26, 2021

UX3D-haertl commented Jan 26, 2021

elalish commented Jan 26, 2021

elalish commented Jan 26, 2021

UX3D-haertl commented Jan 27, 2021

elalish commented Jan 28, 2021

UX3D-haertl commented Jan 29, 2021

elalish commented Jan 29, 2021

elalish commented Jan 29, 2021

UX3D-haertl commented Feb 1, 2021

elalish commented Feb 1, 2021

UX3D-haertl commented Feb 8, 2021

elalish commented Feb 8, 2021

UX3D-haertl commented Feb 10, 2021

elalish commented Feb 11, 2021

elalish commented Feb 11, 2021

UX3D-haertl commented Feb 12, 2021

elalish left a comment

Choose a reason for hiding this comment

UX3D-haertl commented Feb 15, 2021

elalish commented Feb 16, 2021