-
Notifications
You must be signed in to change notification settings - Fork 34
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
[octree] Get neighboring cubes #385
Conversation
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.
Good work!
f9d6e3c
to
9dba059
Compare
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.
Nice work! Just some small things.
I will look into this in the next days, i need some more time for this. |
Summarizing what I already wrote on Discord: |
b61af4a
to
af65a79
Compare
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.
Nice work. I'm not super sure on the octree code, but it's always still fun to review it. One thing to keep in mind though, and this is probably more of a concern for the future, is that having lambdas/heap allocations/lots of shared_ptr
copies in hot code (which I think the octree is/will become since it's at the core of the engine) is generally a bad idea. Having said that, if this function isn't called very often (its main use is for the octree editor or whatever), then it's fine as is.
4644faa
to
aba01ae
Compare
9cd4065
to
1110ed3
Compare
1110ed3
to
706513f
Compare
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 suggest to squash.
706513f
to
a1aaf7b
Compare
and save the index of the cube in the parents array in `Cube::m_child_in_parent`
a1aaf7b
to
e05b548
Compare
Implementing the neighbors method requested in #190 (with a slightly different method signature).
I think we should wait for #368, #262, and #342 before merging this.
Still, it would be great already get feedback on the code. :)
I didn't check it on clang-tidy right now because I expect a lot of changes in this regard with #342.