MeshPhysicalMaterial: Update version for certain transmission changes. #22379
Conversation
That is not exactly true. It depends on the value of |
|
Thanks for the clarification but this does not affect the change. |
|
If we do this we'll have to do the same for |
|
We can probably group them though: if ( material.isMeshPhysicalMaterial ) {
if ( materialProperties.clearcoat !== material.clearcoat > 0 ) {
needsProgramChange = true;
} else if ( materialProperties.sheen !== material.sheen > 0 ) {
needsProgramChange = true;
} else if( materialProperties.transmission !== material.transmission > 0 ) {
needsProgramChange = true;
}
} |
|
It seems
|
|
Actually, I was meant to try this for a while... Seems like we can use Proxy and have this logic in the material instead: class Material {
constructor() {
this.transmission = 0;
this.version = 0;
return new Proxy( this, {
set: function ( target, property, value ) {
if ( property === 'transmission' ) {
if ( target[ property ] > 0 !== value > 0 ) {
target.version ++;
}
}
target[ property ] = value;
return true;
}
} );
}
}const material1 = new Material();
material1.transmission = 1;
console.log( material1.version ); // 1;
const material2 = new Material();
material1.copy( material2 );
console.log( material1.version ); // 2; |
|
Interesting! I clearly favor this approach instead of enhancing |
|
Do you think this could irritate users^^?
|
|
BTW, if I understand correctly a material can support multiple programs now (ex: a program with skinning and another program without skinning). So I expected that if user changes |
|
@takahirox For performance reasons the This circumstance makes it impossible to detect the case reported in #22365. It has to be manually checked in |
Difficulty of refactoring aside, these are expensive effects. We probably do not want them enabled in the shader if they aren't being used... To expand on @takahirox's question a bit, suppose we have three materials in the scene:
If we modify material (2) and set I am a bit nervous of trading an unintuitive "transmission does not update" case for an unintuitive "frame dropped during recompile" case. Knowing that Sorry I believe this was already discussed, but had we already decided against using |
It will reuse (1). There will be no recompile.
I was originally hoping we could opt-out expensive code sections without defines. But it seems there is no way around different shader permutations... |
I don't think we discussed it because I assumed as #22379 (comment) and thought zero or non-zero approach worked well (but it was wrong). |
I think similar to sheen it is more clear when optional shader features are Related #17700 (comment). |
That's annoying indeed... There's this option: class Material {
constructor() {
let transmission = 0;
Object.defineProperty( this, 'transmission', {
enumerable: true,
get: () => {
return transmission;
},
set: ( value ) => {
if ( transmission > 0 !== value > 0 ) {
this.version ++;
}
transmission = value;
}
} );
this.version = 0;
}
}But with this approach
There's also this option: class Material {
#transmission = 0;
constructor() {
this.version = 0;
}
get transmission() {
return this.#transmission;
}
set transmission( value ) {
if ( this.#transmission > 0 !== value > 0 ) {
this.version ++;
}
this.#transmission = value;
}
}But
|
|
I favor the private instance field approach. Besides, we already use getter/setters for certain properties like e.g. |
Mugen87
changed the title
|
Browser support seems to be sufficient, too: https://caniuse.com/?search=private%20class%20fields |
|
Oh! I did not realize Safari 14.1 added support for private class fields. We live in the future! 😁 |
|
Thanks! |
Related issue: Fixed #22365.
Description
If you setup a material with zero transmission and increase it at a later point, nothing will happen. That's because the transmission code path of
MeshPhysicalMaterialis only used whentransmission > 0. A recompile is required whenUSE_TRANSMISSIONshould be defined in the shader (or removed).The only way to detect this with the current transmission implementation is to check each frame the transmission value in
setProgram()and then setneedsProgramChangeif necessary.If we don't want this code section for performance reasons, it's up to the user to set
Material.needsUpdatetotrue. However, I think the behavior reported in #22365 is buggy and should be fixed.