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

2711: do AABB removals by an unordered_map lookup rather than spatial search #2719

Merged
merged 7 commits into from Apr 22, 2019

Conversation

ericwa
Copy link
Collaborator

@ericwa ericwa commented Apr 19, 2019

Fixes #2711

@ericwa ericwa requested a review from kduske April 19, 2019 16:43
@ericwa
Copy link
Collaborator Author

ericwa commented Apr 19, 2019

I haven't tested this for memory leaks yet, and I guess the node heights aren't being tested.
Otherwise I like how this turned out. Now that the oldBounds aren't needed, it paves the way for a batch update API where AABBTree can decide internally whether to make incremental updates or rebuild if enough nodes are changing. Not that it's urgent or anything!

Copy link
Collaborator

@kduske kduske left a comment

Choose a reason for hiding this comment

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

Looks good, very similar to what I had in mind, too. I just have a few minor comments.

common/src/AABBTree.h Outdated Show resolved Hide resolved
common/src/AABBTree.h Outdated Show resolved Hide resolved
common/src/AABBTree.h Outdated Show resolved Hide resolved
common/src/NodeTree.h Show resolved Hide resolved
common/src/AABBTree.h Outdated Show resolved Hide resolved
@ericwa
Copy link
Collaborator Author

ericwa commented Apr 20, 2019

Thanks for the review. One last thing I wanted to do was forward-declare AABBTree and wrap it in a unique_ptr, now you can modify AABBTree and only 2 or 3 files recompile instead of dozens.

@ericwa ericwa added this to the 2019.6 milestone Apr 20, 2019
@ericwa ericwa merged commit d462637 into master Apr 22, 2019
@ericwa ericwa deleted the feature/2711 branch April 22, 2019 22:00
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.

Do AABB removals by the payload, rather than with a spatial search
2 participants