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

Object3D: Add removeAll or clear. #20414

Closed
bianyunzhi95 opened this issue Sep 25, 2020 · 9 comments · Fixed by #20478
Closed

Object3D: Add removeAll or clear. #20414

bianyunzhi95 opened this issue Sep 25, 2020 · 9 comments · Fixed by #20478

Comments

@bianyunzhi95
Copy link

bianyunzhi95 commented Sep 25, 2020

I'd like to have removeAll or clear method in Object3D.

	remove: function ( object ) {

		if ( arguments.length > 1 ) {

			for ( let i = 0; i < arguments.length; i ++ ) {

				this.remove( arguments[ i ] );

			}

			return this;

		}

		const index = this.children.indexOf( object );

		if ( index !== - 1 ) {

			object.parent = null;
			this.children.splice( index, 1 );

			object.dispatchEvent( _removedEvent );

		}

		return this;

	}

remove find child within children with indexOf and remove it using splice which becomes bottleneck when there are a lot of children objects and I need to remove them all which I am experiencing now.

So i suggest to add removeAll method.

	removeAll: function ( ) {

		for ( let i = 0; i < this.children.length; i ++ ) {

			let object = this.children[i];

			object.parent = null;

			object.dispatchEvent( _removedEvent );

		}

		this.children = [];

		return this;

	}
@mrdoob
Copy link
Owner

mrdoob commented Sep 25, 2020

Sounds good to me.

I thing I would prefer something like this:

clear: function () {

	while ( this.children.length > 0 ) {

		const object = this.children.pop();
		object.parent = null;
		object.dispatchEvent( _removedEvent );

	}

	return this;

}

@mrdoob mrdoob added this to the r121 milestone Sep 25, 2020
@mrdoob mrdoob changed the title adding removeAll or clear is useful when there are a lot of children. Object3D: Add removeAll or clear. Sep 25, 2020
@bianyunzhi95
Copy link
Author

bianyunzhi95 commented Sep 25, 2020

this.children.pop spends more time than this.children = [], doesn't it?
Unless this.children is referenced in remove event handler, i think latter one is better for performance. What is your thought?

@mrdoob
Copy link
Owner

mrdoob commented Sep 25, 2020

If a user referenced scene.children somewhere in their code things would break because with this.children = [] it would be a different array.

@bianyunzhi95
Copy link
Author

bianyunzhi95 commented Sep 25, 2020

Then what about this.children.length = 0?

@mrdoob
Copy link
Owner

mrdoob commented Sep 25, 2020

I think that works 👍

@mrdoob
Copy link
Owner

mrdoob commented Sep 25, 2020

Would you like to do a PR with the new method?

@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 8, 2020

In order to get this issue closed, I've filed a PR 😇 .

@mrdoob mrdoob removed this from the r122 milestone Oct 8, 2020
@bianyunzhi95
Copy link
Author

is this gonna added soon or removed forever?

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 18, 2020

#20478 was merged but there are some considerations about the method name. I guess this should be solved before the next release so expect to have this method with r122.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants