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

Small internal refactor of shader variants handling by renderer #4394

Merged
merged 9 commits into from
Jul 1, 2022

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Jul 1, 2022

  • Material.shader used to return either the shader set by the user, or a last generated shader variant (for pass, lights and similar). Now, Material.shader always returns the shader set by the user, or null if shaders are generated (for example by BasicMaterial or StandardMaterial).
  • a material function responsible for generating variants has been renamed from updateShader to getShaderVariant, and instead of updating Material.shader, it returns the generated shader variant, and only updates its internal variants cache.
  • This opens up a path to generate variants even for Material (which use a specified shader), which is required to process the shader to handle uniform buffers (separate PR later).
  • also updated render-to-camera example with a particle system, to test its rendering using multiple-passes

@slimbuck
Copy link
Member

slimbuck commented Jul 1, 2022

Why not call the function getShaderVariant ?

@mvaligursky
Copy link
Contributor Author

Why not call the function getShaderVariant ?

Why not indeed. I'll make it change.

Co-authored-by: Will Eastcott <will@playcanvas.com>
Comment on lines 1330 to 1339
if (!drawCall.isStatic) {
const variantKey = pass + '_' + objDefs + '_' + lightHash;
drawCall._shader[pass] = material.variants[variantKey];
if (!drawCall._shader[pass]) {
this.updateShader(drawCall, objDefs, null, pass, sortedLights);
drawCall.updatePassShader(scene, pass, null, sortedLights);
material.variants[variantKey] = drawCall._shader[pass];
}
} else {
this.updateShader(drawCall, objDefs, drawCall._staticLightList, pass, sortedLights);
drawCall.updatePassShader(scene, pass, drawCall._staticLightList, sortedLights);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-clarity of this blocks also still bothers be, and I'll refactor it later, probably soon as I might need it. I don't like that the caller of updateShader needs to calculate the variant key, and add the shader to material.variants. This should be completely internal ideally, the key computed from shader options generated, and not on some very few variables. I suspect that at the moment, this does not allow us to have all the variations we need, as the keys are the same for them.

Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I can't actually see what material.shader member is used for. Can it just be removed?

src/scene/materials/basic-material.js Outdated Show resolved Hide resolved
src/scene/mesh-instance.js Outdated Show resolved Hide resolved
mvaligursky and others added 2 commits July 1, 2022 13:56
Co-authored-by: Donovan Hutchence <slimbuck7@gmail.com>
Co-authored-by: Donovan Hutchence <slimbuck7@gmail.com>
@mvaligursky
Copy link
Contributor Author

TBH I can't actually see what material.shader member is used for. Can it just be removed?

It's used to assign a user defined shader to the instance of Material class.
It's not used at all for classes derived from Material, so I added some asserts to validate it.

Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is like excavation! Sorry few more questions.

@@ -879,8 +879,9 @@ class ParticleEmitter {
customFace: this.emitter.orientation !== PARTICLEORIENTATION_SCREEN
});
this.shader = shader;
return shader;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous line can (should) be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, I moved it outside of this function, as that's the consistent way to do it.

src/scene/particle-system/particle-emitter.js Outdated Show resolved Hide resolved
src/scene/sky.js Outdated Show resolved Hide resolved
Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more of this code needs refactoring, but this is a step in the right direction. approving.

@mvaligursky mvaligursky merged commit a962f42 into main Jul 1, 2022
@mvaligursky mvaligursky deleted the mvaligursky-getPassShader branch July 1, 2022 14:01
@mvaligursky mvaligursky mentioned this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants