Remove Material Asset on Material change #5109
Conversation
| if (!asset) { | ||
| this.material = null; | ||
| this._materialAsset = _id; |
There was a problem hiding this comment.
this._materialAsset = _id; is in both branches of the if.
Can we replace only 1037 by this._materialAsset = _id || null;?
There was a problem hiding this comment.
The point is that if this._materialAsset is not null when we set this.material to null, this.material setter will call this.materialAsset to null which we don't want. So I had to make sure it's null before assigning to the right value after processing with material reset.
It's also not possible to change this._material directly as it won't reset the displayed material.
So yeah a bit upside-down but I couldn't find any better way to do it...
The other solution could be to switch to:
this._materialAsset = null;
this.material = null;
this._materialAsset = _id
There was a problem hiding this comment.
@querielo Any further comments here or is this particular conversation resolved?
There was a problem hiding this comment.
I moved to the other solution
this._materialAsset = null;
this.material = null;
this._materialAsset = _id;
So it's more clear that the null assignment is only for material nulling and reduces the number of changed lines, as it's only to reduces call from material setter and avoid a loop between the two.
In fact the bug was coming from material setter not changing the _materialAsset and the enable callback using the _materialAsset if any, overriding any existing _material if it was from a different ID/no ID. So the main point is to remove _materialAsset inside material setter which was then making a loop with materialAsset setter if _materialAsset was non-null or different, why the null.
|
@mvaligursky I think you should be the one to hit the merge button if you're satisfied. Over to you... 😄 |
|
It looks good, merging in, thanks all! |
| if (this._materialAsset) { | ||
| const asset = this._system.app.assets.get(this._materialAsset); | ||
| if (!asset || asset.resource !== value) { | ||
| this.materialAsset = null; |
There was a problem hiding this comment.
Should this line be this._materialAsset = null? as calling the setter for materialAsset also sets the _material to null as well? It's also a circular call where the setter for materialAsset calls this material setter.
Or does it still need to remove the listeners on the asset as well? In which case, we need to create separate function to remove the event listeners
|
This PR seems to be causing a problem reported by a forum user: They're cloning the material on an element and replacing the element's material with the clone. This internally sets materialAsset = null which in turn sets material to null. I'm wondering if we should back this out. Got any thoughts @MushAsterion? |
|
The fix in #5327 has not been merged to a release yet @willeastcott |
|
You're right! With so many patch releases, I was sure this was out already. |
Fixes #3560
Set
ImageElement#materialAssettonullonmaterialchange if new material does not matchmaterialAssetto avoid overwrite fromImageElement#onEnable. Important to use setter instead of_materialAssetto remove listeners.I confirm I have read the contributing guidelines and signed the Contributor License Agreement.