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

Fix Node Materials Not Compiling #16668

Merged
merged 3 commits into from
Jun 20, 2019
Merged

Conversation

DanielSturk
Copy link
Contributor

@DanielSturk DanielSturk commented Jun 3, 2019

I've been using @sunag's awesome node graph system plenty as of late, and have stumbled across another bug

Testing:

  • Checkout the 1st commit (dca1fac "Broke webgl_materials_nodes.html to expose bug")
  • Load up webgl_materials_nodes.html
  • Switch to bump material from the drop down
  • The 'color' checkbox won't do anything anymore
    --- The 2nd commit will fix the bug
    --- The 3rd commit will undo the first commit (exposing the bug)

Explanation of cause:
77: NodeMaterial.prototype.onBeforeCompile belongs to the prototype, so all instances of NodeMaterial share it (per the exact same subclass, eg. PhongNodeMaterial, since each subclass duplicates the prototype)
39: this.onBeforeCompile.toString treats onBeforeCompile as being uniquely instanced for each class, but actually when you override onBeforeCompile.toString it overrides it for all instances
Therefore, all instances of the same subclass will share the same, most recent instance of onBeforeCompile.toString, which is used by WebGLRenderer to decide whether or not the recompile is actually needed

@DanielSturk
Copy link
Contributor Author

DanielSturk commented Jun 3, 2019

Also I don't think this is a great way of triggering the recompile, because

  1. It's unorthodox to override the toString of a function, eg. it obscures the actual function body in the debug tools (you still can through onBeforeCompile.prototype.constructor I'd think?)
  2. It's tightly coupled with the timing of WebGLRenderer calling Material.onBeforeCompile/WebGLPrograms.getProgramCode

That being said, it's not a heavily used feature so I understand why it doesn't justify major code change

@sunag
Copy link
Collaborator

sunag commented Jun 3, 2019

I think we can do a little different this time about .toString.

// remove this

var self = this;
this.onBeforeCompile.toString = ...

replace needsCompile (set/get)

this.needsCompile = value;

to this.defines["NEEDS_COMPILE"]

this.defines["NEEDS_COMPILE"] = value;

and add this.needsUpdate = false in onBeforeCompile:

onBeforeCompile = function ( shader, renderer ) {

	if ( this.needsUpdate ) {

		this.build( { renderer: renderer } );

		shader.uniforms = this.uniforms;
		shader.vertexShader = this.vertexShader;
		shader.fragmentShader = this.fragmentShader;

		this.needsUpdate = false;

	}

};

I did not test but I think it should work.

@sunag
Copy link
Collaborator

sunag commented Jun 3, 2019

I think that this.defines["NEEDS_UPDATE"] instead of this.defines["NEEDS_COMPILE"].

@sunag
Copy link
Collaborator

sunag commented Jun 3, 2019

To improve this, it is necessary to improve the hash generator in the core.

@sunag
Copy link
Collaborator

sunag commented Jun 3, 2019

About core, I think all this string generate in getProgramCode should be a checksum thinking about reducing memory usage and string conditional.

I also think that we should improve that part specifically, which is the cause of NodeMaterial use this hack. getProgramCode does not consider shader modification in onBeforeCompile() hash. only in the modification of the code of own function.

array.push( material.onBeforeCompile.toString() );

@sunag sunag mentioned this pull request Jun 16, 2019
@mrdoob mrdoob merged commit 4e96336 into mrdoob:dev Jun 20, 2019
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.

3 participants