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.

Sign up for GitHub

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

BufferAttribute: Make normalize() and denormalize() internal functions #24512

Closed
wants to merge 4 commits into from
Closed

BufferAttribute: Make normalize() and denormalize() internal functions #24512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93eb9ab
MathUtils: Rename normalize() → floatToInt(), denormalize() → intToFloat
donmccurdy Aug 18, 2022
64f0c46
BufferGeomtetryUtils: Remove redundant normalization.
donmccurdy Aug 19, 2022
3e71793
BufferAttribute: Clean up.
donmccurdy Aug 19, 2022
16cfc1f
BufferAttribute: Limit exports from src/Three.js
donmccurdy Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions examples/jsm/utils/BufferGeometryUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
InstancedBufferAttribute,
InterleavedBuffer,
InterleavedBufferAttribute,
MathUtils,
TriangleFanDrawMode,
TriangleStripDrawMode,
TrianglesDrawMode,
Expand Down Expand Up @@ -36,17 +35,16 @@ function computeMikkTSpaceTangents( geometry, MikkTSpace, negateSign = true ) {

if ( attribute.normalized || attribute.isInterleavedBufferAttribute ) {

const srcArray = attribute.isInterleavedBufferAttribute ? attribute.data.array : attribute.array;
const dstArray = new Float32Array( attribute.getCount() * attribute.itemSize );

for ( let i = 0, j = 0; i < attribute.getCount(); i ++ ) {

dstArray[ j ++ ] = MathUtils.denormalize( attribute.getX( i ), srcArray );
dstArray[ j ++ ] = MathUtils.denormalize( attribute.getY( i ), srcArray );
dstArray[ j ++ ] = attribute.getX( i );
dstArray[ j ++ ] = attribute.getY( i );

if ( attribute.itemSize > 2 ) {

dstArray[ j ++ ] = MathUtils.denormalize( attribute.getZ( i ), srcArray );
dstArray[ j ++ ] = attribute.getZ( i );

}

Expand Down
14 changes: 13 additions & 1 deletion src/Three.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,19 @@ export { InstancedInterleavedBuffer } from './core/InstancedInterleavedBuffer.js
export { InterleavedBuffer } from './core/InterleavedBuffer.js';
export { InstancedBufferAttribute } from './core/InstancedBufferAttribute.js';
export { GLBufferAttribute } from './core/GLBufferAttribute.js';
export * from './core/BufferAttribute.js';
export {
Float64BufferAttribute,
Float32BufferAttribute,
Float16BufferAttribute,
Uint32BufferAttribute,
Int32BufferAttribute,
Uint16BufferAttribute,
Int16BufferAttribute,
Uint8ClampedBufferAttribute,
Uint8BufferAttribute,
Int8BufferAttribute,
BufferAttribute
} from './core/BufferAttribute.js';
export { Object3D } from './core/Object3D.js';
export { Raycaster } from './core/Raycaster.js';
export { Layers } from './core/Layers.js';
Expand Down
184 changes: 151 additions & 33 deletions src/core/BufferAttribute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import { Vector4 } from '../math/Vector4.js';
import { Vector3 } from '../math/Vector3.js';
import { Vector2 } from '../math/Vector2.js';
import { Color } from '../math/Color.js';
import { denormalize, normalize } from '../math/MathUtils.js';
import { StaticDrawUsage } from '../constants.js';
import { ByteType, FloatType, IntType, ShortType, StaticDrawUsage, UnsignedByteType, UnsignedIntType, UnsignedShortType } from '../constants.js';

const _vector = /*@__PURE__*/ new Vector3();
const _vector2 = /*@__PURE__*/ new Vector2();
Expand All @@ -27,6 +26,8 @@ class BufferAttribute {
this.count = array !== undefined ? array.length / itemSize : 0;
this.normalized = normalized === true;

this.type = getArrayType( array );

Comment on lines +29 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the value of this field?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to avoid this property and just evaluate the typed array in normalize()/denormalize()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think passing the entire array into normalize/denormalize as a huge 'type' argument was never ideal as a public MathUtils API... if this went back into MathUtils at some point, I think it is better to use types like IntType / ShortType. But since this is just internal, I suppose it does not matter so much. Any preference between:

  • (A) limit visibility of the property, e.g. __type or #type
  • (B) continue passing the array

/cc @WestLangley

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer B for now until there's a practical reason to change it. It makes it hard to see what the other meaningful changes are in this PR, as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH, I prefer any form of (A). Passing the array is horrible, IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

imo passing a three.js enum can result in unnecessarily verbose user code with no value. If something like "A" is used I'd prefer to pass in something like the array type / constructor: normalize( value, Int8Array ).

A lot of the code I've been working with has been written to generally support array types within three. For example packing buffers into textures for three-gpu-pathtracer, generating data structures in three-mesh-bvh, etc. A lot of the code in three.js isn't designed to support these types of use cases which is fine but it would be nice to keep it in mind when adding new functionality to the system - these use cases have been the driver for #24515.

Copy link
Collaborator Author

@donmccurdy donmccurdy Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What if we keep these functions accessible through THREE.MathUtils, but rename them much more explicitly, and export getArrayType(array) as well. Then usage might look something like:

import { MathUtils } from 'three';

const i = 127;
const type = MathUtils.getArrayType( Uint8Array );

const f = MathUtils.decodeNormalizedInt( i, type );
const i2 = MathUtils.encodeNormalizedInt( f, type );

I think this naming might be less likely to confuse people, as it's a more explicit about the use of integers as an encoding of floating-point values, per the OpenGL definition:

A Normalized Integer is an integer which is used to store a decimal floating point number...

Copy link
Collaborator

@gkjohnson gkjohnson Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm still not fully understanding the need for a separate set of "type" constants. I guess I'm just not understanding the need to redefine constants that the language already has built in with the typed array constructors. It just feels a bit roundabout and redundant - similar to the Float32BufferAttribute, Uint8BufferAttribute, etc classes. But I won't fight it if it's what the project wants.

this.usage = StaticDrawUsage;
this.updateRange = { offset: 0, count: - 1 };

Expand Down Expand Up @@ -58,6 +59,8 @@ class BufferAttribute {
this.count = source.count;
this.normalized = source.normalized;

this.type = source.type;

this.usage = source.usage;

return this;
Expand Down Expand Up @@ -105,9 +108,9 @@ class BufferAttribute {

if ( this.normalized ) {

array[ offset ++ ] = normalize( color.r, array );
array[ offset ++ ] = normalize( color.g, array );
array[ offset ++ ] = normalize( color.b, array );
array[ offset ++ ] = denormalize( color.r, this.type );
array[ offset ++ ] = denormalize( color.g, this.type );
array[ offset ++ ] = denormalize( color.b, this.type );

} else {

Expand Down Expand Up @@ -141,8 +144,8 @@ class BufferAttribute {

if ( this.normalized ) {

array[ offset ++ ] = normalize( vector.x, array );
array[ offset ++ ] = normalize( vector.y, array );
array[ offset ++ ] = denormalize( vector.x, this.type );
array[ offset ++ ] = denormalize( vector.y, this.type );

} else {

Expand Down Expand Up @@ -175,9 +178,9 @@ class BufferAttribute {

if ( this.normalized ) {

array[ offset ++ ] = normalize( vector.x, array );
array[ offset ++ ] = normalize( vector.y, array );
array[ offset ++ ] = normalize( vector.z, array );
array[ offset ++ ] = denormalize( vector.x, this.type );
array[ offset ++ ] = denormalize( vector.y, this.type );
array[ offset ++ ] = denormalize( vector.z, this.type );

} else {

Expand Down Expand Up @@ -211,10 +214,10 @@ class BufferAttribute {

if ( this.normalized ) {

array[ offset ++ ] = normalize( vector.x, array );
array[ offset ++ ] = normalize( vector.y, array );
array[ offset ++ ] = normalize( vector.z, array );
array[ offset ++ ] = normalize( vector.w, array );
array[ offset ++ ] = denormalize( vector.x, this.type );
array[ offset ++ ] = denormalize( vector.y, this.type );
array[ offset ++ ] = denormalize( vector.z, this.type );
array[ offset ++ ] = denormalize( vector.w, this.type );

} else {

Expand Down Expand Up @@ -311,7 +314,11 @@ class BufferAttribute {

set( value, offset = 0 ) {

if ( this.normalized ) value = normalize( value, this.array );
if ( this.normalized && Array.isArray( value ) ) {

throw new TypeError( 'THREE.BufferAttribute: array should be a Typed Array.' );

}
donmccurdy marked this conversation as resolved.
Show resolved Hide resolved

this.array.set( value, offset );

Expand All @@ -323,15 +330,15 @@ class BufferAttribute {

let x = this.array[ index * this.itemSize ];

if ( this.normalized ) x = denormalize( x, this.array );
if ( this.normalized ) x = normalize( x, this.type );

return x;

}

setX( index, x ) {

if ( this.normalized ) x = normalize( x, this.array );
if ( this.normalized ) x = denormalize( x, this.type );

this.array[ index * this.itemSize ] = x;

Expand All @@ -343,15 +350,15 @@ class BufferAttribute {

let y = this.array[ index * this.itemSize + 1 ];

if ( this.normalized ) y = denormalize( y, this.array );
if ( this.normalized ) y = normalize( y, this.type );

return y;

}

setY( index, y ) {

if ( this.normalized ) y = normalize( y, this.array );
if ( this.normalized ) y = denormalize( y, this.type );

this.array[ index * this.itemSize + 1 ] = y;

Expand All @@ -363,15 +370,15 @@ class BufferAttribute {

let z = this.array[ index * this.itemSize + 2 ];

if ( this.normalized ) z = denormalize( z, this.array );
if ( this.normalized ) z = normalize( z, this.type );

return z;

}

setZ( index, z ) {

if ( this.normalized ) z = normalize( z, this.array );
if ( this.normalized ) z = denormalize( z, this.type );

this.array[ index * this.itemSize + 2 ] = z;

Expand All @@ -383,15 +390,15 @@ class BufferAttribute {

let w = this.array[ index * this.itemSize + 3 ];

if ( this.normalized ) w = denormalize( w, this.array );
if ( this.normalized ) w = normalize( w, this.type );

return w;

}

setW( index, w ) {

if ( this.normalized ) w = normalize( w, this.array );
if ( this.normalized ) w = denormalize( w, this.type );

this.array[ index * this.itemSize + 3 ] = w;

Expand All @@ -405,8 +412,8 @@ class BufferAttribute {

if ( this.normalized ) {

x = normalize( x, this.array );
y = normalize( y, this.array );
x = denormalize( x, this.type );
y = denormalize( y, this.type );

}

Expand All @@ -423,9 +430,9 @@ class BufferAttribute {

if ( this.normalized ) {

x = normalize( x, this.array );
y = normalize( y, this.array );
z = normalize( z, this.array );
x = denormalize( x, this.type );
y = denormalize( y, this.type );
z = denormalize( z, this.type );

}

Expand All @@ -443,10 +450,10 @@ class BufferAttribute {

if ( this.normalized ) {

x = normalize( x, this.array );
y = normalize( y, this.array );
z = normalize( z, this.array );
w = normalize( w, this.array );
x = denormalize( x, this.type );
y = denormalize( y, this.type );
z = denormalize( z, this.type );
w = denormalize( w, this.type );

}

Expand Down Expand Up @@ -599,6 +606,114 @@ class Float64BufferAttribute extends BufferAttribute {

//

function getArrayType( array ) {

switch ( array.constructor ) {

case Uint32Array:

return UnsignedIntType;

case Uint16Array:

return UnsignedShortType;

case Uint8Array:
case Uint8ClampedArray:

return UnsignedByteType;

case Int32Array:

return IntType;

case Int16Array:

return ShortType;

case Int8Array:

return ByteType;

case Float32Array:
case Float64Array:

return FloatType;

default:

throw new Error( 'THREE.BufferAttribute: Invalid type.' );

}

}

function normalize( value, type ) {

switch ( type ) {

case FloatType:

return value;

case UnsignedShortType:

return value / 65535.0;

case UnsignedByteType:

return value / 255.0;

case ShortType:

return Math.max( value / 32767.0, - 1.0 );

case ByteType:

return Math.max( value / 127.0, - 1.0 );

default:

throw new Error( 'THREE.BufferAttribute: Invalid type.' );

}

}

function denormalize( value, type ) {

switch ( type ) {

case FloatType:

return value;

case UnsignedShortType:

return Math.round( value * 65535.0 );

case UnsignedByteType:

return Math.round( value * 255.0 );

case ShortType:

return Math.round( value * 32767.0 );

case ByteType:

return Math.round( value * 127.0 );

default:

throw new Error( 'THREE.BufferAttribute: Invalid type.' );

}

}

//

export {
Float64BufferAttribute,
Float32BufferAttribute,
Expand All @@ -610,5 +725,8 @@ export {
Uint8ClampedBufferAttribute,
Uint8BufferAttribute,
Int8BufferAttribute,
BufferAttribute
BufferAttribute,
normalize,
denormalize,
getArrayType
};
Loading