Skip to content
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

WebGPURenderer: Fix NodeLibrary Logic for Minified Builds #29501

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented Sep 26, 2024

Related issue: #29187

Description

Issue:

Minifiers mangle class names like MeshStandardMaterial.name to shorter identifiers (e.g., a2), causing errors such as:

NodeMaterial: Material "MeshStandardMaterial" is not compatible.

Solution:

This PR fixes the issue by introducing a static type property on material classes. By using materialClass.type instead of materialClass.name, we prevent minification from breaking material compatibility checks.

PS:
Note: I strongly recommend merging this or a similar fix before releasing r169, as this issue currently breaks all Vite applications using the WebGPURenderer.

This contribution is funded by Utsubo

Copy link

github-actions bot commented Sep 26, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 687.46
170.22
687.77
170.3
+305 B
+77 B
WebGPU 840.72
225.49
841.02
225.55
+305 B
+65 B
WebGPU Nodes 840.23
225.36
840.53
225.43
+305 B
+66 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 463.24
111.86
463.36
111.91
+123 B
+47 B
WebGPU 534.87
144.3
535.13
144.32
+263 B
+23 B
WebGPU Nodes 491.21
134.05
491.31
134.1
+95 B
+43 B

@RenaudRohlinger RenaudRohlinger changed the title WebGPURenderer: Fix NodeLibrary after minification WebGPURenderer: Fix NodeLibrary Logic for Minified Builds Sep 26, 2024
@RenaudRohlinger RenaudRohlinger marked this pull request as ready for review September 26, 2024 07:08

}

set type( _value ) { /* */ }
Copy link
Collaborator Author

@RenaudRohlinger RenaudRohlinger Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of this but since we're using es2018 we can't do:

class Material extends EventDispatcher {
    static type = 'MeshBasicMaterial';
    ...
}

and:

class Material extends EventDispatcher {
    ...
}
Material.type = 'Material';

would break treeshaking I believe.

@RenaudRohlinger RenaudRohlinger added this to the r169 milestone Sep 26, 2024
@mrdoob mrdoob modified the milestones: r169, r170 Sep 26, 2024
@donmccurdy
Copy link
Collaborator

Let's merge this and get it into r170, if there are no objections — It would be really nice to support bundlers with WebGPU, blocked by this and #29156.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 1, 2024

@RenaudRohlinger Just a heads up since I've almost overlooked the post of @FarazzShaikh in the commit: ac963e2#r148635345

We might want to partly revert this PR by keeping the type member so it can be easier updated on app level. Does that sound good to you?

Granted, it is supposed to be a read-only property and there is an easy workaround (ac963e2#r148640959) but I wonder if it's better to not confront users with a migration task by restoring the previous code.

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.

4 participants