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.

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: Fix the behavior of ".set" function #24511

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #22874

Description

The BufferAttribute.set function takes an array not a scalar value.

cc @donmccurdy

@gkjohnson gkjohnson requested a review from donmccurdy August 18, 2022 22:10
@gkjohnson gkjohnson added this to the r144 milestone Aug 18, 2022
@donmccurdy
Copy link
Collaborator

donmccurdy commented Aug 18, 2022

Hmmm good catch here, thanks. I think we will either decide what the type of the input array is supposed to be, or else to handle a few edge cases.

Consider a normalized Uint8Array BufferAttribute.

  • if 'value' array is Uint8Array, normalize() loop will clamp to [0,1] breaking the result. Perhaps no conversion is expected?
  • if 'value' array is Float32Array or number[], is conversion expected?

I'd also point out that set( array, offset ) and copyArray( array ) are nearly identical except that the latter does not take an offset and applies no normalization conversions.

Given that the input array to the BufferAttribute was presumably a Uint8Array containing integers, I wonder if we should just assume any array inputs to these methods will not require further conversion? Or I suppose we could compare the type of the array vs. the internal array. 😕

@gkjohnson
Copy link
Collaborator Author

I think we will either decide what the type of the input array is supposed to be, or else to handle a few edge cases.

I think it will get confusing if we handle anything like this. I think its okay to assume that the explicit values being passed into the function are the ones the user is trying to use - ie treat every array as a regular, non typed array as this PR does.

I'd also point out that set( array, offset ) and copyArray( array ) are nearly identical except that the latter does not take an offset and applies no normalization conversions.

I'm okay with adding "offset" to "copyArray". But I do think that if "set" remains it needs to normalize / denormalize values since the naming aligns with the other setter / getter functions

@donmccurdy
Copy link
Collaborator

donmccurdy commented Aug 19, 2022

But I do think that if "set" remains it needs to normalize / denormalize values ...

Could you say more about why you expect that? By another convention (THREE.Color, THREE.VectorX) our .set( ... ) methods usually take the same value types as their class constructor. I guess I am worried that this case will confuse people:

const array = new Uint8Array( [ 0, 64, 0, ... ] );

const attribute = new BufferAttribute( array.slice(), 3, true );
attribute.getY( 0 ); // 0.25

attibute.set( array.slice() );
attribute.getY( 0 ); // 0.0

If we are going to drop support for u8, i8, u16, i16, u32, and i32 arrays in this method, I think we would need to include some warnings or errors rather than doing this silently.

@gkjohnson
Copy link
Collaborator Author

Hm you're right. I suppose there's just an inconsistency now from either perspective. The other BufferAttribute.set* functions handle normalizing / denormalizing so a user may expect .set to do the same. But other classes use it as you've described. I can't say which I prefer or think users would be thinking about it more consistently.

But either way this isn't what I intended to address in this PR. I'm happy to change it to whatever is preferred. I don't feel strongly enough about the behavior here to spend too much time discussing it.

@donmccurdy
Copy link
Collaborator

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 20, 2022

I would prefer to merge this PR so the regression is fixed. Let's discuss a change in behavior elsewhere.

@Mugen87 Mugen87 merged commit 1995a1e into mrdoob:dev Aug 22, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 22, 2022

Merging to include this PR in r144.

@donmccurdy
Copy link
Collaborator

I think it would be best if .set( ... ) did not perform any (de)normalization at all. This PR will break the attribute if given an array in the same format as was used to construct the attribute. I'll open a new PR for this, in case #24512 requires more time.

@donmccurdy
Copy link
Collaborator

Followup: #24526

snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 13, 2022
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants