Skip to content

Commit

Permalink
WebGPURenderer: Fix NodeLibrary Logic for Minified Builds (#29501)
Browse files Browse the repository at this point in the history 
* WebGPURenderer: Fix NodeLibrary after minification

* add warning

* fix missing materials

* fix depthmaterial, thanks e2e tests

* some minifier moves type as property
  • Loading branch information
@RenaudRohlinger
RenaudRohlinger authored Sep 27, 2024
1 parent f9480de commit ac963e2
Show file tree
Hide file tree
Showing 23 changed files with 142 additions and 42 deletions.
9 changes: 7 additions & 2 deletions examples/jsm/lines/LineMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,12 +405,17 @@ ShaderLib[ 'line' ] = {

class LineMaterial extends ShaderMaterial {


static get type() {

return 'LineMaterial';

}

constructor( parameters ) {

super( {

type: 'LineMaterial',

uniforms: UniformsUtils.clone( ShaderLib[ 'line' ].uniforms ),

vertexShader: ShaderLib[ 'line' ].vertexShader,
Expand Down
6 changes: 6 additions & 0 deletions examples/jsm/loaders/LDrawLoader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ const _tempVec1 = new Vector3();

class LDrawConditionalLineMaterial extends ShaderMaterial {

static get type() {

return 'LDrawConditionalLineMaterial';

}

constructor( parameters ) {

super( {
Expand Down
8 changes: 6 additions & 2 deletions examples/jsm/loaders/MMDLoader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2128,14 +2128,18 @@ class CubicBezierInterpolation extends Interpolant {

class MMDToonMaterial extends ShaderMaterial {

static get type() {

return 'MMDToonMaterial';

}

constructor( parameters ) {

super();

this.isMMDToonMaterial = true;

this.type = 'MMDToonMaterial';

this._matcapCombine = AddOperation;
this.emissiveIntensity = 1.0;
this.normalMapType = TangentSpaceNormalMap;
Expand Down
8 changes: 6 additions & 2 deletions examples/jsm/materials/MeshGouraudMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,14 +315,18 @@ const GouraudShader = {

class MeshGouraudMaterial extends ShaderMaterial {

static get type() {

return 'MeshGouraudMaterial';

}

constructor( parameters ) {

super();

this.isMeshGouraudMaterial = true;

this.type = 'MeshGouraudMaterial';

//this.color = new THREE.Color( 0xffffff ); // diffuse

//this.map = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/LineBasicMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@ import { Color } from '../math/Color.js';

class LineBasicMaterial extends Material {

static get type() {

return 'LineBasicMaterial';

}

constructor( parameters ) {

super();

this.isLineBasicMaterial = true;

this.type = 'LineBasicMaterial';

this.color = new Color( 0xffffff );

this.map = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/LineDashedMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@ import { LineBasicMaterial } from './LineBasicMaterial.js';

class LineDashedMaterial extends LineBasicMaterial {

static get type() {

return 'LineDashedMaterial';

}

constructor( parameters ) {

super();

this.isLineDashedMaterial = true;

this.type = 'LineDashedMaterial';

this.scale = 1;
this.dashSize = 3;
this.gapSize = 1;
Expand Down
15 changes: 14 additions & 1 deletion src/materials/Material.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,20 @@ let _materialId = 0;

class Material extends EventDispatcher {

static get type() {

return 'Material';

}

get type() {

return this.constructor.type;

}

set type( _value ) { /* */ }

This comment has been minimized.

Sign in to view

Copy link
@FarazzShaikh

FarazzShaikh Nov 1, 2024

Contributor

Hello @RenaudRohlinger @Mugen87 (sorry for the ping)

This looks like a big breaking change to me. Some libraries like my three-custom-shader-material set the type manually so their custom onBeforeCompile wrappers remain transparent to ThreeJS' internals.

I am sure other libs in the eco system do the same. I am already seeing erros that may be related to this with troika-three-text using r170

This change is not mentioned in the migration guide and is very hard to spot

This comment has been minimized.

Sign in to view

Copy link
@Mugen87

Mugen87 Nov 1, 2024 •
edited
Loading

Collaborator

We did not anticipate that users actually change the type. This also stated in the documentation:

Value is the string 'Material'. This shouldn't be changed, and can be used to find all objects of this type in a scene.

I'm not sure what's best now. What is exactly breaking when type isn't available as a writable property anymore? Could you fall back to a custom property?

This comment has been minimized.

Sign in to view

Copy link
@FarazzShaikh

FarazzShaikh Nov 1, 2024

Contributor

Yes i totally understand that the docs tell us not to set type, what I and others are doing is non-standard usage

Correct me if im wrong, but ThreeJS uses the type property to set the default uniforms for its in-built materials. In my case, my library lets users create materials that are wrappers around onBeforeCompile and this this wrapper class needs be the "transparent" to ThreeJS's internals so that the correct default uniforms are set on it.

So if the user sets the base mateiral as MeshPhysicalMaterial, my extenstion class needs to be processed just like a MeshPhysicalMaterial by the internals

Without setting the type, ThreeJS does not set the required default uniforms on the extended class.

I imagine other librarirs like troika also use type for a similar purpose

If this change is non-essential to any other feature, I would revert it in the next release. Yes, it will be another breaking change reverting this but not many of us use type anyway and it would likely go unnoticed

Else, I would add this to the migration guide (if its editable) so people have a heads up

As a workaround, I have overidden then getter and setter in JS. I imageine others could do the same if we decide not to revert. This override is as stated bellow

class CustomMaterial extends Material {
  constructor(baseMaterial) {
    super()
    Object.defineProperty(this, "type", {
      get() {
        return baseMaterial.type;
      },
      set(value) {
        baseMaterial.type = value;
      },
    });
  }
}

This comment has been minimized.

Sign in to view

Copy link
@Mugen87

Mugen87 Nov 1, 2024 •
edited
Loading

Collaborator

I have updated the migration guide for r170 with a respective note.

Yes, you are right about the usage of type. It is used in WebGLPrograms to identify the correct shader source and uniforms for a given material:

three.js/src/renderers/webgl/WebGLPrograms.js

Line 573 in 841ca14

const shaderID = shaderIDs[ material.type ];

three.js/src/renderers/webgl/WebGLPrograms.js

Line 58 in 841ca14

const shaderID = shaderIDs[ material.type ];

I'm glad you have found a workaround to make things work again in your project.

@RenaudRohlinger Could we fix this by keeping the static type property for NodeLibrary but restore the previous type property? I guess this should not interfere with node materials and the use case of @FarazzShaikh will work again.

This comment has been minimized.

Sign in to view

Copy link
@FarazzShaikh

FarazzShaikh Nov 1, 2024

Contributor

Thanks for the resolution guys. Please let me know if we decide not to revert, I will put out a message to troika maintainers and other folk I know who use this property.

Cheers! 🙌

This comment has been minimized.

Sign in to view

Copy link
@lojjic

lojjic Nov 2, 2024

Contributor

@FarazzShaikh Thanks for your analysis, you were exactly correct and my quick fix/workaround in troika-three-utils 0.50.3 was exactly the same as yours.

This wasn't an issue of attempting to change the type property, but expecting it to behave as a simple instance property when prototype-chaining, which was no longer the case because it was given a dependency on a new static that wasn't accounted for. 🤷

Anyway, it looks like it's being reverted, which will unbreak older versions.

constructor() {

super();
Expand All @@ -18,7 +32,6 @@ class Material extends EventDispatcher {
this.uuid = MathUtils.generateUUID();

this.name = '';
this.type = 'Material';

this.blending = NormalBlending;
this.side = FrontSide;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshBasicMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ import { Euler } from '../math/Euler.js';

class MeshBasicMaterial extends Material {

static get type() {

return 'MeshBasicMaterial';

}

constructor( parameters ) {

super();

this.isMeshBasicMaterial = true;

this.type = 'MeshBasicMaterial';

this.color = new Color( 0xffffff ); // emissive

this.map = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshDepthMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@ import { BasicDepthPacking } from '../constants.js';

class MeshDepthMaterial extends Material {

static get type() {

return 'MeshDepthMaterial';

}

constructor( parameters ) {

super();

this.isMeshDepthMaterial = true;

this.type = 'MeshDepthMaterial';

this.depthPacking = BasicDepthPacking;

this.map = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshDistanceMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@ import { Material } from './Material.js';

class MeshDistanceMaterial extends Material {

static get type() {

return 'MeshDistanceMaterial';

}

constructor( parameters ) {

super();

this.isMeshDistanceMaterial = true;

this.type = 'MeshDistanceMaterial';

this.map = null;

this.alphaMap = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshLambertMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@ import { Euler } from '../math/Euler.js';

class MeshLambertMaterial extends Material {

static get type() {

return 'MeshLambertMaterial';

}

constructor( parameters ) {

super();

this.isMeshLambertMaterial = true;

this.type = 'MeshLambertMaterial';

this.color = new Color( 0xffffff ); // diffuse

this.map = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshMatcapMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ import { Color } from '../math/Color.js';

class MeshMatcapMaterial extends Material {

static get type() {

return 'MeshMatcapMaterial';

}

constructor( parameters ) {

super();
Expand All @@ -13,8 +19,6 @@ class MeshMatcapMaterial extends Material {

this.defines = { 'MATCAP': '' };

this.type = 'MeshMatcapMaterial';

this.color = new Color( 0xffffff ); // diffuse

this.matcap = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshNormalMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,18 @@ import { Vector2 } from '../math/Vector2.js';

class MeshNormalMaterial extends Material {

static get type() {

return 'MeshNormalMaterial';

}

constructor( parameters ) {

super();

this.isMeshNormalMaterial = true;

this.type = 'MeshNormalMaterial';

this.bumpMap = null;
this.bumpScale = 1;

Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshPhongMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@ import { Euler } from '../math/Euler.js';

class MeshPhongMaterial extends Material {

static get type() {

return 'MeshPhongMaterial';

}

constructor( parameters ) {

super();

this.isMeshPhongMaterial = true;

this.type = 'MeshPhongMaterial';

this.color = new Color( 0xffffff ); // diffuse
this.specular = new Color( 0x111111 );
this.shininess = 30;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshPhysicalMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ import * as MathUtils from '../math/MathUtils.js';

class MeshPhysicalMaterial extends MeshStandardMaterial {

static get type() {

return 'MeshPhysicalMaterial';

}

constructor( parameters ) {

super();
Expand All @@ -18,8 +24,6 @@ class MeshPhysicalMaterial extends MeshStandardMaterial {

};

this.type = 'MeshPhysicalMaterial';

this.anisotropyRotation = 0;
this.anisotropyMap = null;

Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshStandardMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ import { Euler } from '../math/Euler.js';

class MeshStandardMaterial extends Material {

static get type() {

return 'MeshStandardMaterial';

}

constructor( parameters ) {

super();
Expand All @@ -14,8 +20,6 @@ class MeshStandardMaterial extends Material {

this.defines = { 'STANDARD': '' };

this.type = 'MeshStandardMaterial';

this.color = new Color( 0xffffff ); // diffuse
this.roughness = 1.0;
this.metalness = 0.0;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/MeshToonMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ import { Color } from '../math/Color.js';

class MeshToonMaterial extends Material {

static get type() {

return 'MeshToonMaterial';

}

constructor( parameters ) {

super();
Expand All @@ -13,8 +19,6 @@ class MeshToonMaterial extends Material {

this.defines = { 'TOON': '' };

this.type = 'MeshToonMaterial';

this.color = new Color( 0xffffff );

this.map = null;
Expand Down
8 changes: 6 additions & 2 deletions src/materials/PointsMaterial.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@ import { Color } from '../math/Color.js';

class PointsMaterial extends Material {

static get type() {

return 'PointsMaterial';

}

constructor( parameters ) {

super();

this.isPointsMaterial = true;

this.type = 'PointsMaterial';

this.color = new Color( 0xffffff );

this.map = null;
Expand Down
Loading

0 comments on commit ac963e2

Please sign in to comment.