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

Clarify Loop end condition in BufferGeometry.merge() #15827

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

takahirox
Copy link
Collaborator

Comment inline.

@@ -822,7 +822,7 @@ BufferGeometry.prototype = Object.assign( Object.create( EventDispatcher.prototy

var attributeSize = attribute2.itemSize;

for ( var i = 0, j = attributeSize * offset; i < attributeArray2.length; i ++, j ++ ) {
for ( var i = 0, j = attributeSize * offset; i < attributeArray2.length && j < attributeArray1.length; i ++, j ++ ) {

attributeArray1[ j ] = attributeArray2[ i ];
Copy link
Collaborator Author

@takahirox takahirox Feb 22, 2019

Choose a reason for hiding this comment

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

(Update: Clean up the comment)

This method seems to expect attributeArray1 size won't be resized.

j can be bigger than the attributeArray1.length here but attributeArray1/2 are typed array then attributeArray1[ index ] = value; // index >= attributeArray1.length has no effect.

I think better to make clear the behavior by clarifying loop end condition. And it'll be efficient in case j can be bigger than attributeArray1.length because it can cut off no effect operation .

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Because assigning to an out-of-bounds index in a typed array has no effect and throws no error, this change has no effect on the method output, and just makes the actual behavior more explicit right? Sounds good to me.

@takahirox
Copy link
Collaborator Author

Yes, that's right. Thanks for clarifying.

@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2019

Would this be more clear?

var attributeOffset = attribute2.itemSize * offset;
var length = Math.min( attributeArray2.length, attributeArray1.length - attributeOffset );

for ( var i = 0, j = attributeOffset; i < length; i ++, j ++ ) {

	attributeArray1[ j ] = attributeArray2[ i ];

}

@mrdoob mrdoob modified the milestones: r102, r103 Feb 27, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@mrdoob
Copy link
Owner

mrdoob commented Apr 24, 2019

/ping @takahirox

@takahirox
Copy link
Collaborator Author

Sorry for the late. Updated.

@mrdoob mrdoob merged commit d0dbcd7 into mrdoob:dev Apr 24, 2019
@mrdoob
Copy link
Owner

mrdoob commented Apr 24, 2019

Thanks!

@takahirox takahirox deleted the BufferGeometryEndCondition branch April 24, 2019 08:54
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