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

[octree] Get neighboring cubes #385

Merged
merged 1 commit into from
Jul 1, 2021
Merged

[octree] Get neighboring cubes #385

merged 1 commit into from
Jul 1, 2021

Conversation

movabo
Copy link
Collaborator

@movabo movabo commented May 4, 2021

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.

@movabo movabo added cat:enhancement enhancement/requested feature/update of existing features prio:low This has low priority. feat:octree octree, cube computations org:review changes are finished, review in progress or requested labels May 4, 2021
@movabo movabo self-assigned this May 4, 2021
@IAmNotHanni IAmNotHanni added this to In progress in Octree-editor via automation May 4, 2021
Copy link
Member

@IAmNotHanni IAmNotHanni left a comment

Choose a reason for hiding this comment

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

Good work!

@movabo movabo force-pushed the movabo/cube_neighbor branch 4 times, most recently from f9d6e3c to 9dba059 Compare May 6, 2021 16:42
@movabo movabo marked this pull request as ready for review May 6, 2021 16:46
Copy link
Member

@IAmNotHanni IAmNotHanni left a 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.

src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
@IceflowRE
Copy link
Member

I will look into this in the next days, i need some more time for this.
Just for now, iam not a huge fan of the cached index value.

@movabo
Copy link
Collaborator Author

movabo commented May 6, 2021

Summarizing what I already wrote on Discord:
I'm not a fan of it either. However I could not come up with a solution without knowing the index.
And then, I think it's better to keep track of the index than again-and-again iterating over the children-array of the parent to find the index.
Also, my guess is that it might be worth to save the index in a variable for the future.

@movabo movabo force-pushed the movabo/cube_neighbor branch 2 times, most recently from b61af4a to af65a79 Compare May 7, 2021 18:22
@IAmNotHanni IAmNotHanni changed the title Get neighboring cubes [octree] Get neighboring cubes May 16, 2021
Copy link
Member

@yeetari yeetari left a 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.

include/inexor/vulkan-renderer/world/cube.hpp Outdated Show resolved Hide resolved
include/inexor/vulkan-renderer/world/cube.hpp Outdated Show resolved Hide resolved
include/inexor/vulkan-renderer/world/cube.hpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
src/vulkan-renderer/world/cube.cpp Outdated Show resolved Hide resolved
@movabo movabo force-pushed the movabo/cube_neighbor branch 6 times, most recently from 4644faa to aba01ae Compare June 26, 2021 20:56
@movabo movabo requested review from IceflowRE and yeetari June 26, 2021 21:02
@movabo movabo force-pushed the movabo/cube_neighbor branch 3 times, most recently from 9cd4065 to 1110ed3 Compare June 26, 2021 22:51
@movabo movabo requested a review from IceflowRE June 27, 2021 12:23
Copy link
Member

@IceflowRE IceflowRE left a 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.

and save the index of the cube in the parents array in `Cube::m_child_in_parent`
@movabo movabo merged commit c45d3bf into master Jul 1, 2021
Octree-editor automation moved this from In progress to Done Jul 1, 2021
@movabo movabo deleted the movabo/cube_neighbor branch July 1, 2021 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement enhancement/requested feature/update of existing features feat:octree octree, cube computations org:review changes are finished, review in progress or requested prio:low This has low priority.
Projects
Octree-editor
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants