Skip to content

TSL: Fix instance() usage with velocity. #30846

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

Merged
merged 1 commit into from
Apr 2, 2025
Merged

TSL: Fix instance() usage with velocity. #30846

merged 1 commit into from
Apr 2, 2025

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 2, 2025

Related issue: -

Description

I'm currently doing some deeper testing with VelocityNode. Turns out the node does not work with instancing yet since positionPrevious has no valid value per instance so far. The PR fixes that by honoring the instance matrix in positionPrevious.

Copy link

github-actions bot commented Apr 2, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.36
78.34
336.36
78.34
+0 B
+0 B
WebGPU 533.07
148.15
533.11
148.16
+35 B
+13 B
WebGPU Nodes 532.54
148.05
532.57
148.06
+35 B
+13 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 465.37
112.21
465.37
112.21
+0 B
+0 B
WebGPU 604.68
164.12
604.72
164.13
+35 B
+14 B
WebGPU Nodes 559.67
153.56
559.71
153.57
+35 B
+10 B

@sunag sunag added this to the r176 milestone Apr 2, 2025
@sunag
Copy link
Collaborator

sunag commented Apr 2, 2025

@Mugen87 Do you think we can use the TSL prefix and names in the title instead of the Nodes names? It seems to be easier for the end user to see the fixes and improvements of TSL in each release.

@Mugen87 Mugen87 changed the title InstanceNode: Fix usage with VelocityNode. instance(): Fix usage with velocity. Apr 2, 2025
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 2, 2025

Yes, we can do that. That also means the release notes need a new TSL section that lists such changes.

@Mugen87 Mugen87 merged commit 998ee48 into mrdoob:dev Apr 2, 2025
12 checks passed
@sunag
Copy link
Collaborator

sunag commented Apr 2, 2025

Yes, we can do that. That also means the release notes need a new TSL section that lists such changes.

Yes, good idea.
What do you think about using this pattern for now?

TSL: Fix instance() usage with velocity.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 2, 2025

LGTM! 👍

@Mugen87 Mugen87 changed the title instance(): Fix usage with velocity. TSL: Fix instance() usage with velocity. Apr 2, 2025
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 3, 2025

Doing some more testing and I've realized this fix isn't 100% correct. To get the real positionPrevious when using instancing, we have to apply the instanceMatrixNode version from the previous frame. Similar to SkinningNode, we need a pendant for previousBoneMatricesNode. So previousInstanceMatrixNode and all instance matrices from the previous frame.

Besides, we need this in all modules that apply any kind of vertex transformation. E.g.: MorphNode also needs a fix otherwise the velocity isn't correct and breaks anything using it like Motion Blur and TRAA.

Um, maintaining all these "previous" data is not really a scalable solution. When users modulate positionLocal in their app, they automatically break velocity computation.

Mugen87 added a commit that referenced this pull request Apr 3, 2025
Mugen87 added a commit that referenced this pull request Apr 3, 2025
@sunag
Copy link
Collaborator

sunag commented Apr 3, 2025

E.g.: MorphNode also needs a fix otherwise the velocity isn't correct and breaks anything using it like Motion Blur and TRAA.

We should calculate the vertexes in the compute stage, similar to what was done in this example. I think it should improve performance as well, and it is the general solution for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants