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

Make SurfaceReconstruction an inout param #7

Merged

Conversation

whiterabbit42k
Copy link
Contributor

Allows passing own SurfaceReconstruction in new
function that reuses vectors and indices.

The density map is still recalculated for now.

@w1th0utnam3
Copy link
Member

Looks good from a quick skim, I'll check it in detail when I have a bit more free time.

splashsurf_lib/src/lib.rs Outdated Show resolved Hide resolved
splashsurf_lib/src/density_map.rs Outdated Show resolved Hide resolved
splashsurf_lib/src/uniform_grid.rs Outdated Show resolved Hide resolved
splashsurf_lib/src/mesh.rs Show resolved Hide resolved
Allows passing own SurfaceReconstruction in new
function that reuses vectors and indices.

The density map is still recalculated for now, but is
now optional.
@whiterabbit42k
Copy link
Contributor Author

ok addressed concerns from last review, except the non-derived default, as noted in comment above.

@w1th0utnam3
Copy link
Member

Great thanks.

I think for now it won't change much in performance, but it's a starting point. I think it would make sense to go through the code and try to minimize allocations or to try to reuse memory. E.g. by adding the neighborhood list to the output and reusing its memory. However, I'll first focus on reworking the pipeline to use some kind of domain decomposition followed by replacing the hashmaps. But I think it will take some time before it can reach your performance requirements 😅

@w1th0utnam3 w1th0utnam3 merged commit 31c0bfe into InteractiveComputerGraphics:master Dec 28, 2020
@whiterabbit42k
Copy link
Contributor Author

Yup agree, I don't think it's a huge bump at the moment; i'm wondering if reusing the density map will be but that wasn't apart of this PR.

I also noticed almost all the functions are marked inline(never); I assume this was for profiling reasons? If yes, perhaps we can cfg them out (with the profile feature?)

@whiterabbit42k whiterabbit42k deleted the inout branch December 29, 2020 07:44
@w1th0utnam3 w1th0utnam3 added the enhancement New feature or request label Dec 16, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants