API Audit: Shadertools.getUniforms() prevUniforms behavior is unclear

[felixpalmer]

Background

The ShaderUniforms.getUniforms() function has a parameter, prevUniforms which is seemingly used differently depending on the context. Either it is passed the populated uniforms obtained from dependent modules (dependent mode), or it is passed the currently set uniforms (cached mode).

Dependent mode

In assembleGetUniforms, getUniforms is invoked on modules in dependency order, passing the already obtained uniforms to prevUniforms:

luma.gl/modules/shadertools/src/lib/shader-assembly/assemble-shaders.ts

Lines 435 to 440 in f63579c

	for (const module of modules) {
	// `modules` is already sorted by dependency level. This guarantees that
	// modules have access to the uniforms that are generated by their dependencies.
	const moduleUniforms = module.getUniforms?.(opts, uniforms);
	Object.assign(uniforms, moduleUniforms);
	}

Cached mode

In ShaderInputs, getUniforms is invoked only on the modules present in the setProps call, without computing dependencies, and here prevUniforms contains just the value of the currently set uniforms for the module in question:

luma.gl/modules/engine/src/shader-inputs.ts

Lines 112 to 114 in f63579c

	const oldUniforms = this.moduleUniforms[moduleName];
	const oldBindings = this.moduleBindings[moduleName];
	let uniformsAndBindings = module.getUniforms?.(moduleProps, this.moduleUniforms[moduleName]);

Expected behavior

The behavior should be consistent and better documented.

@ibgreen could you confirm what your intent here was with the API? The Dependent mode seems like the more logical design, as it would easily allow modules to obtain uniforms from their dependencies (as for example we could do here: https://github.com/visgl/deck.gl/blob/5ce18d5c04ec015b3fc558402cbd3e6287196c9c/modules/extensions/src/terrain/shader-module.ts#L118)

To obtain the currently set values, we already have ShaderInputs.getUniformValues()

[ibgreen]

@ibgreen could you confirm what your intent here was with the API?

Totally agree that this API must be firmed up and documented.

cached - Originally this was simply done so that one should be able to call shaderModule.getUniforms() multiple times, with different subset of props (not having to pass all the props every time). I suppose this is what you call the cached mode. As long as the original props are not needed this works, but also made some of the getUniforms implementations more complex than I liked.

dependent - I did not consider a case where modules should be able to access or change the uniforms of their dependencies. I am somewhat wary of it - it seems we are looking at 1 use case only? Also in this case we are accessing a computed uniform, but we are doing this by recomputing the project uniforms, the props for which must now be passed in somehow when calling this.

[felixpalmer]

call shaderModule.getUniforms() multiple times

Do we have any examples where the prevUniforms get used when props are missing?

I did not consider a case where modules should be able to access or change the uniforms of their dependencies

Seems to be intended in the past: #379

it seems we are looking at 1 use case only?

There are a couple other examples, however I agree it isn't widespread, and pulling in the dependent module explictly doesn't seem bad to me and gives the flexibility to pass different values in the props.

I do wonder if it would be best to remove the prevUniforms parameter altogether as we move to UBO

[ibgreen]

Do we have any examples where the prevUniforms get used when props are missing?

Without checking. I would have thought that they are always used by the global getShaderModuleUniforms() function to return a "full" uniforms object (i.e. there is a function that is supposed to be called instead of calling this function directly on the modules).

As for using them as inputs to recalculate new uniforms from partial props, I think this was done in the post process effects, one of lighting parameter modules most likely.

[ibgreen]

There are a couple other examples, however I agree it isn't widespread, and pulling in the dependent module explictly doesn't seem bad to me and gives the flexibility to pass different values in the props.

Side track: Exploring symmetry with the actual shader code...

For shader code I have been going in the direction of exposing any module values via APIs instead of having dependent modules "rake" into the modules internal uniforms, which by definition are internal (the entire purpose of getUniforms is to map long-lived stable public props to internal uniforms that can change freely as the module's implementation is refactored).

So in this case we would have perhaps project_getCommonOrigin() function exported from the project module in GLSL.

Then a question is whether having a symmetric API also on the JS side makes sense, or if in JS it is OK to access the uniforms.

[felixpalmer]

Having just ported the shadow module which requires 3 uniforms from project I don't think it is simply a matter of exposing the values to the shader code. The uniform values need to be available to the JavaScript code.

I don't find the pattern of invoking project.getUniforms to be a problem, so I think I'm leaning towards restricting only supporting the cached use case - it is also better for clear types

[ibgreen]

The uniform values need to be available to the JavaScript code.

OK... this means that the shader module "props abstraction" is only partial. It may protect simpler apps from uniform changes, but will still require any dependent modules (possibly defined in applications) to be modified when shader module uniforms change.

It also seems a little messy to have to pass around all the props for multiple modules so that one module can call getUniforms on other modules.

An alternative that the module provides an API for JS, similar to an API for shaders, but that is a little more involved.

I don't find the pattern of invoking project.getUniforms to be a problem

Should we recommend using the global function like getShaderModuleUniforms(project, ...) instead of directly calling the getUniforms() field, so that we can do some additional module initialization and default processing, or should we drop that function and rely on possibly more code in the shader modules. Don't have a strong opinion yet.

[ibgreen]

Note: The above are just considerations, I am fine with the direction you are proposing.