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

OctreeHelper: Add update(). #24641

Merged
merged 7 commits into from
Sep 19, 2022
Merged

OctreeHelper: Add update(). #24641

merged 7 commits into from
Sep 19, 2022

Conversation

erasta
Copy link
Contributor

@erasta erasta commented Sep 14, 2022

Related issue: #23481

When Octree changes, you need to recreate another OctreeHelper. This PR adds an update() method to rebuild the visualization on the same geometry.

@Mugen87 Mugen87 added this to the r145 milestone Sep 15, 2022
@Mugen87 Mugen87 changed the title Add OctreeHelper.update() OctreeHelper: Add update(). Sep 15, 2022
@WestLangley
Copy link
Collaborator

The update() method is calling dispose() in this PR.

TBH, I'm thinking that adding a dispose() method would be more appropriate than adding an update() method here.

This class can't be simply updated -- a new instance is generally required.

It is the app's responsibility to call dispose() when appropriate.

At least, that has been our policy.

Without a consistently-implemented policy, users will never know when calling dispose() is required at the app level.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 16, 2022

This class can't be simply updated -- a new instance is generally required.

I've also thought about this although other helpers do have update methods. I'm okay for both approaches in context of OctreeHelper.

@erasta
Copy link
Contributor Author

erasta commented Sep 16, 2022

The update() method is calling dispose() in this PR.
TBH, I'm thinking that adding a dispose() method would be more appropriate than adding an update() method here.
This class can't be simply updated -- a new instance is generally required.
It is the app's responsibility to call dispose() when appropriate.
At least, that has been our policy.
Without a consistently-implemented policy, users will never know when calling dispose() is required at the app level.

Some helpers have a dispose(), more helpers have an update() method, and others have some variant of it as ArrowHelper.SetDirection().
I agree with you that there should be a dispose() method, and more than that:
All helpers should implement both dispose() and update() methods, either with regulation on code reviews, or by enforcing it using some mechanism such as base class.
(However, in this PR i didn't want to rock the boat)

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 16, 2022

I don't think that something like AxesHelper needs an update() method.

@WestLangley
Copy link
Collaborator

This class can't be simply updated -- a new instance is generally required.

I've also thought about this although other helpers do have update methods.

But the update methods just update the buffer data.

In this case, I think an update() method is unwise, for the reasons I stated above.

I would prefer to see a dispose() method added, instead.

@erasta
Copy link
Contributor Author

erasta commented Sep 17, 2022

Added dispose() method that also disposes of the material, as other helpers do.
Kept the update() method, because sometimes it's beneficial to retain the object reference instead of disposing and recreating it. Hope this is ok with you.
There's a recursion there that i want to change into a loop, but not sure if this is for separate PR.
Thank you.

@Mugen87 Mugen87 merged commit ee3b5ad into mrdoob:dev Sep 19, 2022
@joshuaellis joshuaellis mentioned this pull request Sep 27, 2022
17 tasks
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