-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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(). #20478
Object3D: Add removeAll(). #20478
Conversation
I find |
Thanks! |
@@ -314,6 +314,11 @@ <h3>[method:this remove]( [param:Object3D object], ... )</h3> | |||
Removes *object* as child of this object. An arbitrary number of objects may be removed. | |||
</p> | |||
|
|||
<h3>[method:this removeAll]()</h3> | |||
<p> | |||
Removes all child objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeAll()
. Remove all what?
Does it remove the object? The object's children? The children of the children?
And how are they disposed?
This perhaps should be an app-level method, not a library method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes all children, but doesn't traverse the hierarchy.
I've found myself needing this pretty often, but yes... the dispose situation can be confusing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it belongs in the library. The proper implementation is app-specific.
But at a minimum, I think the name should be changed to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should probably change remove()
to removeChild()
, too. Or event to removeChildButNotChildrenOfChild()
...
TBH, I think the name (and the method itself) is totally fine. Improving the documentation to provide more clarity is of course always a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But at a minimum, I think the name should be changed to something else.
Neither Mugen87 nor I are native english speakers. Any help with naming is always welcome 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to go with .clear()
which is what I used in ui.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, this conflicts with CubeCamera
:
https://github.com/mrdoob/three.js/blob/dev/src/cameras/CubeCamera.js#L97-L111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also breaks the TS build:
src/cameras/CubeCamera.d.ts(16,2): error TS2416: Property 'clear' in type 'CubeCamera' is not assignable to the same property in base type 'Object3D'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'll think about this.
Related issues:
Fixed #20414.
Description
I like
removeAll()
more than theclear()
since the relation toremove()
is more obvious. Or in other words, it should be easier for user to figure out whatremoveAll()
does if they are familiar with the semantics ofremove()
.