BufferAttribute: Support (de)normalization in accessors (v2) #24087
Conversation
| @@ -22,7 +23,10 @@ class BufferAttribute { | |||
| this.array = array; | |||
| this.itemSize = itemSize; | |||
| this.count = array !== undefined ? array.length / itemSize : 0; | |||
| this.normalized = normalized === true; | |||
|
|
|||
There was a problem hiding this comment.
I think this is not needed because the original line will perform the same steps in the setter?
| @@ -222,7 +241,9 @@ class BufferAttribute { | |||
|
|
|||
| for ( let i = 0, l = this.count; i < l; i ++ ) { | |||
|
|
|||
| _vector.fromBufferAttribute( this, i ); | |||
| _vector.x = this.getX( i ); | |||
There was a problem hiding this comment.
I think it is better to fix Vector.fromBufferAttribute...
There was a problem hiding this comment.
No, it still works.
Then I don't think this line (and lines similar to this) should be changed?
There was a problem hiding this comment.
Indeed. Inlining fromBufferAttribute() only makes the code more verbose.
|
Thanks for the comments @LeviPesin! I'll wait to address feedback until we've agreed on a direction between this and #22874. |
|
Wild guess says this PR might fix this bug: google/model-viewer#3577 I like this approach; @mrdoob any feelings one way or the other? Would be nice to get this merged. |
|
The setter/getter interface of |
More compact code, using internal
._normalize(value)and._denormalize(value)helper functions, created to match the underlying array type. Requires turning .normalized into a getter/setter. Making .normalized readonly would also be an option.