-
Notifications
You must be signed in to change notification settings - Fork 816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add 3 anisotropy tests as suggested by EricChadwick #4493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -94,15 +94,15 @@ Alternative error: | |||
! was unexpected at this time. | |||
npm ERR! code ELIFECYCLE | |||
npm ERR! errno 1 | |||
npm ERR! @google/model-viewer@1.10.1 prepare: `if [ ! -L './shared-assets' ]; then ln -s ../shared-assets ./shared-assets; fi && ../shared-assets/scripts/fetch-khronos-gltf-samples.sh` | |||
npm ERR! @google/model-viewer@1.10.1 prepare: `if [ ! -L './shared-assets' ]; then ln -s ../shared-assets ./shared-assets; fi && ../shared-assets/scripts/fetch-khronos-gltf-models.sh && && ../shared-assets/scripts/fetch-khronos-gltf-assets.sh` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we take this opportunity to switch from the old sample-models repo to the new sample-assets repo instead of using both? The models are the same anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be okay if I switch from this custom git checkout method to the more standard git submodule method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, that sounds great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: #4534
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do babylon/filament/sample-viewer support anisotropy yet? If so, we should update to latest versions and make new goldens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was commenting on one of them, which clearly doesn't support anisotropy. My question is: if we update our package.json to their latest versions, do they already support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a new PR here (#4535) to bring in the anisotropy tests here on top of my other PR. I've updated Babylon and Filament to the latest versions as well. Let me know if this meets you requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any idea what's causing those scrollbars to appear? I've seen them randomly for ages, but can't seem to repro on my machine. I assume it's some CSS bug somewhere... They're annoying because they mess up the metrics (not that they're terribly reliable anyway, but still).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a nice example of why rendering transparency to a transparent canvas shouldn't be ignored...
Closing this PR as it has been made obsolete by these two new PRs: |
Reference Issue
Fixes the missing anisotropy tests mentioned in this issue: #4486
This involved: