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

Suggestion: Support refreshing specific uniforms independent of switching program / material #9870

Closed
3 of 12 tasks
fallenoak opened this issue Oct 17, 2016 · 17 comments
Closed
3 of 12 tasks

Comments

@fallenoak
Copy link
Contributor

fallenoak commented Oct 17, 2016

Description of the problem

onBeforeRender was added as a solution for #9662

Unfortunately, because of the way setProgram works in THREE.WebGLRenderer, uniform changes made in the onBeforeRender callback can end up ignored. For example, if the render loop attempts to draw two objects with the same material, and the second material has a uniform that was updated by onBeforeRender, refreshMaterial will fail to be set to true, and the uniform will not be refreshed.

This was highlighted by @arose here: #9662 (comment)

I've given a shot at supporting this kind of uniform change on a branch here: fallenoak@24c5d9a

I'd have preferred calling the THREE.Uniform flag needsUpdate, but it seems like that's already used for something (not sure what, however).

I'd also have liked a less kludgy way of iterating over defined uniforms for a given material. Perhaps there's a better way of doing this (still a bit new to how uniforms are managed in THREE).

Apologies on recycling the 'dynamic' label for these uniforms. It does seem like an appropriate name to me.

Three.js version
  • Dev
  • r81
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • Linux
  • Android
  • IOS
Hardware Requirements (graphics card, VR Device, ...)
@fallenoak
Copy link
Contributor Author

@mrdoob Would love your feedback when you have time. Happy to craft a PR once you weigh in.

@mrdoob
Copy link
Owner

mrdoob commented Oct 18, 2016

Heh... We seem to be going back and forth here... 65b8e84

@fallenoak
Copy link
Contributor Author

fallenoak commented Oct 18, 2016

I hope this isn't too back-and-forth-y! I think onBeforeRender is fantastic compared to the previous dynamic uniforms mechanism:

  • onBeforeRender and onAfterRender are much more general purpose than dynamic uniforms were, and facilitate many more use cases, while remaining intuitive for per object uniform changes.
  • onBeforeRender makes it possible to accomplish multiple uniform changes per object in a single function call, instead of a function call per uniform per object. This dramatically lowers the number of function calls when multiple uniforms are 'dynamic'.

All that's really missing is an appropriate way to mark a uniform as dirty, such that the setProgram logic will ensure the value gets uploaded. Options that come to mind are:

  • Marking a uniform as dynamic, such that its value always gets uploaded (ie is always considered dirty). From what I understand, this is how dynamic uniforms were handled before.
  • Leaving the decision to mark a uniform as dirty up to user-defined logic in onBeforeRender. This might be a bit too flexible / prone to causing confusion for users.
  • Adding some kind of auto flagging mechanism. setProgram could mark a uniform as uploaded when the program / material switch happens, and if onBeforeRender modifies the uniform value before the next program / material switch, some logic could reflag the uniform as in need of an upload. This might be too magical and/or suffer from poor performance.

@fallenoak
Copy link
Contributor Author

@mrdoob Thoughts?

@arose
Copy link
Contributor

arose commented Apr 20, 2017

Finally figured out how to use onBeforeRender to update uniforms:

function onBeforeRender( renderer, scene, camera, geometry, material, group ){

	var updateList = [];
	var u = material.uniforms;
	var d = this.userData;

	if( u.pickingColor ){
		u.pickingColor.value.setHex( d.pickingColor );
		updateList.push( "pickingColor" );
	}

	if( u.color ){
		u.color.value.setHex( d.color );
		updateList.push( "color" );
	}

	if( updateList.length ){

		var materialProperties = renderer.properties.get( material );

		if( materialProperties.program ){

			var gl = renderer.getContext();
			var p = materialProperties.program;
			gl.useProgram( p.program );
			var pu = p.getUniforms();

			updateList.forEach( function( name ){
				pu.setValue( gl, name, u[ name ].value );
			} );

		}

	}

}

What about another callback in WebGLRenderer that is called after setProgram is called so that the call to gl.useProgram is not necessary and object.modelViewMatrix is already updated?

@bodison
Copy link

bodison commented Apr 21, 2017

@arose
I am doing something similar, but I did not want to use useProgram there. That's really only necessary for the first object you render, during later calls of onBeforeRender the program is already bound and you can set the uniforms no problem. So instead I set the material uniforms for the first mesh, THREE then sets the uniforms itself after it binds the program.

	function onBeforeRender(renderer, scene, camera, geometry, material, group) {

		if ( !material.programIsSet ) {
 			const map = material.uniforms;
			map.ufSaturation.value = this.userData.saturation;
			material.programIsSet = true;
			return;
		}
		if ( material.program && material.uniforms !== undefined ) {
			const gl = material.program.getUniforms().renderer.context;
			const map = material.program.getUniforms().map;
			map.ufSaturation.setValue(gl, this.userData.saturation);
		}
	}

@arose
Copy link
Contributor

arose commented Apr 21, 2017

@bodison the program can change, so you need to set which to use as far as I can tell.

@bodison
Copy link

bodison commented Apr 22, 2017

@arose true. Mine is not a robust solution, I really should not use this material again after switching to a different one in my pipeline. (or reset programIsSet, but who wants to keep track of that...)

@pailhead
Copy link
Contributor

pailhead commented Apr 26, 2017

We should probably try to do something so that this kind of code doesn't have to be so gnarly :(
Ideally just changing the uniform on the material should reflect the change, without any low level calls, but it requires a bit of an overhaul?

@arose

Would it make any sense to somehow put this logic into Uniform's domain? Like an explicit method to set the uniform for the currently bound program, although i've no idea how or if that would work :/, it would probably only be valid to use within this callback?

@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2018

I added a uniformsNeedUpdate flag to ShaderMaterial in d4b3dab while refactoring Lensflare.
It may also solve this issue. Anyone up for giving it a go?

@mrdoob mrdoob added this to the rXX milestone Feb 27, 2018
@TheJim01
Copy link

Don't know if this is relevant anymore, but uniformsNeedUpdate made this work exactly as expected. (Tested in r92.) Thanks!

@zacaj
Copy link
Contributor

zacaj commented Mar 14, 2019

uniformsNeedUpdate is also fixing an issue for me when using onBeforeRender. But I don't see it in the docs or in the typescript types. Is this the accepted way to update uniforms for shaders, or is there some other way this should be done?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 14, 2019

Related #15581

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 11, 2020

Docs should be added with the above PR. The TS file was already updated here: #18289

It's important to highlight that uniformsNeedUpdate is only available for ShaderMaterial. It's not possible to use this option for other built-in materials like MeshStandardMaterial.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 17, 2021

Since there is a solution for ShaderMaterial, I think this issue can be closed.

I suggest you follow #21305 for an alternative, more generic approach. And there is already an elegant solution with the new node material, see webgpu_instance_uniform (WebGPU only right now).

@Mugen87 Mugen87 closed this as completed Mar 17, 2021
@WestLangley
Copy link
Collaborator

Remove milestone?

@Mugen87 Mugen87 removed this from the rXXX milestone Mar 17, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 17, 2021

Thanks for the pointer! Done.

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

No branches or pull requests

9 participants