LineMaterial: Clean up #26720
LineMaterial: Clean up #26720
Conversation
|
|
||
| }, | ||
| if ( ! this.uniforms.linewidth ) return; |
There was a problem hiding this comment.
Can you explain the reason for this line, please?
There was a problem hiding this comment.
Sure -- the problem is that in the ShaderMaterial contructor linewidth property is set, so the setter is called, so if this.uniforms.linewidth is undefined (as it is in that constructor), assign to a property of undefined fails. A cleaner solution would be to use optional chaining.
There was a problem hiding this comment.
With all due respect, I feel uneasy about the four lines I highlighted. If I had a better solution, I would suggest it.
Maybe comments would help.
@Mugen87 has good judgement about this sort of thing. I will defer to whatever he thinks.
There was a problem hiding this comment.
I think the best solution is to use optional chaining or at least do something like if ( this.uniforms && this.uniforms.linewidth ) this.uniforms.linewidth.value = ....
There was a problem hiding this comment.
Since there are many contributors to three.js, it is important that we write code that can be maintained by others. IMHO, the lines I highlighted here are too hacky. It appears you agree there are better approaches.
I would be satisfied reverting to the prior implementation.
|
|
||
| this.uniforms.resolution.value.copy( value ); | ||
| if ( ! this.uniforms ) return; | ||
| this.uniforms.opacity.value = value; |
There was a problem hiding this comment.
Can you explain the reason for this line, please?
There was a problem hiding this comment.
Same as above, but opacity is set in Material.
|
|
||
| if ( value === true ) { | ||
| if ( ! this.defines ) return; |
There was a problem hiding this comment.
Can you explain this line, too, please?
|
|
||
| this.defines.USE_ALPHA_TO_COVERAGE = ''; | ||
| this.extensions.derivatives = true; | ||
| if ( ( value === true ) !== this.alphaToCoverage ) { |
There was a problem hiding this comment.
Maybe this line, too...
There was a problem hiding this comment.
This line is almost the same as was before, I just simplified it a bit.
Related issue: #26704 (comment)
Description
Replace Object.defineProperties() with class fields getters/setters.