-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 UV2 morph support and load UV and UV2 morph targets from glTF #15602
base: master
Are you sure you want to change the base?
Conversation
To reviewer: I'm not sure how to test the code changes. I only know it works because I have a downstream application that uses node material. Haven't tested other materials at all. |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
ae8fe37
to
d000b83
Compare
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/15602/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/15602/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/15602/merge#BCU1XR#0 |
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.
The code looks really solid at a first glance, I ll let @deltakosh validate and @bghgary for the gltf part
@chubei-urus can you add a gltf visualization test ? so it would not experience nme but already a big part of the code. |
Sure @sebavan! Do you have any pointers on where to add that? |
Visualization tests for WebGPU (Experimental) |
you can add a test model in https://github.com/BabylonJS/Assets and then follow the contributing doc here to add the new test https://doc.babylonjs.com/contribute/toBabylon/HowToContribute#visualization-tests Do not hesitate to provide doc feedback so we can improve it if needed :-) |
WebGL2 visualization test reporter: |
Visualization tests for WebGL 1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
Seems like some WGSL shader updates are missing, is this PR still a draft? |
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.
We need to also update all the missing wgsl shaders
I didn't know how to run the vis tests locally. I'll convert to draft and fix the errors before requesting review again. |
Looks good from the glTF perspective. The spec says this FYI:
|
@chubei-urus |
Hey there! any update? |
Hi! I haven't found time to work on this PR yet. |
This pull request has been marked as stale because it has been inactive for more than 14 days. Please update to "unstale". |
d000b83
to
1c4778d
Compare
Visualization tests for WebGPU (Experimental) |
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.
LGTM!
You will also have to update:
https://github.com/BabylonJS/Babylon.js/blob/master/packages/dev/core/src/Meshes/mesh.ts#L3962-L4026
Yes, we can keep the PR in draft until you add a proper visu test. |
I'm opening this PR following the suggestion in https://forum.babylonjs.com/t/morph-uv2-and-import-the-morph-target-from-gltf/53558/2.
The PR adds uv2 morphing to the built-in materials as well as the morph target block in the node material. It also modifies the glTF loader to load the TEXCOORD_0 and TEXCOORD_1 morph target attributes.