WebGLMaterials helper

[taphos]

Introduced WebGLMaterials helper
made all WebgLRenderer helpers customizable via WebGLRendererParameters

[taphos]

#19292

[Mugen87]

I've tested the PR on my local computer and it seems to work fine.

@mrdoob I have one (minor) concern about this PR: WebGLMaterials does not encapsulates the entire logic for updating uniforms. It is now spread across WebGLMaterials and WebGLRenderer which could make the code harder to follow. The renderer still has to evaluate material parameters in setProgram() to ensure correct uniforms in the shader. So WebGLMaterials is a first step but the change feels a bit "incomplete".

[taphos]

change feels a bit "incomplete".

This is my first PR, just to understand how you guys work. I will be happy to help making WebGLRenderer less monolithic, messy and more flexible. If you allow me to.

[mrdoob]

@mrdoob I have one (minor) concern about this PR: WebGLMaterials does not encapsulates the entire logic for updating uniforms. It is now spread across WebGLMaterials and WebGLRenderer which could make the code harder to follow. The renderer still has to evaluate material parameters in setProgram() to ensure correct uniforms in the shader. So WebGLMaterials is a first step but the change feels a bit "incomplete".

Thanks. Yes, I think this is definitely one step in the right direction. I think more refactoring will become clearer after this.

[mrdoob]

Hmm, you changed the order of this code. Before the PR it executed after WebGLUniforms.upload() and now it's executing before.

I'll merge the PR but I'll put this code back just in case. Step by step...

[Mugen87]

AFAICS, that should not have any side effects. It's just important that this bit comes before this section:

if ( material.isShaderMaterial && material.uniformsNeedUpdate === true ) {

    WebGLUniforms.upload( _gl, materialProperties.uniformsList, m_uniforms, textures );
    material.uniformsNeedUpdate = false;

}

Otherwise WebGLUniforms.upload() is called twice for ShaderMaterials when uniformsNeedUpdate is set to true.

[taphos]

ok. This makes me wonder, how much you guys trust unit and e2e tests?
And how safe is more radical refactoring?

[Mugen87]

This makes me wonder, how much you guys trust unit and e2e tests?

Right now, the test coverage is not sufficient so broad refactoring can only be done with a lot of manual testing.

[mrdoob]

ok. This makes me wonder, how much you guys trust unit and e2e tests?

Unit tests and e2e are nice to have but they wouldn't catch uploading uniforms twice.

The codebase is still fairly small just so we can still keep it in our heads (more or less). I would hate if we get to a point where no one understands it and we just rely on tests.

And how safe is more radical refactoring?

Generally not a fan of radical refactorings. Incremental refactoring are more manageable.

[mrdoob]

Okay, moved the code back: b063139

[mrdoob]

Thanks!