Add experimental improved RSR and Hierarchical Reconstruction #139
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
Contributor
Author
|
It seems there is a problem in the original RSR code when trying to reconstruct using the points of the Armadillo (as.obj). On my end, it just infinitely loops instead of throwing bad_alloc. Not sure what to do about this. |
Owner
Weird. It works on my computer. But the result is very bad unless I provide normals. Where does it break for you? I will just take it up with Ruiqi |
Owner
|
Ruiqi and I just looked at it. We cannot reproduce the infinite loop, so it would be great with some indication of where it happens? However, it is clear that the normal estimation does not work well for synthetic models. Ruiqi and I will look into improving the normal estimate such that it also gives good results for as.obj - and similar models. Just to be clear, the RSR reconstruction in master works perfectly on as.obj if normals computed in MeshLab are provided. |
Contributor
Author
|
Sorry, I misread the test logs. It is not the armadillo, it is thingy.obj that is causing issues. It crashes on the CI and infinite loops on my machine. |
Owner
|
Right. I think it is the normal computation that is to blame. I got the result below, but with normals computed using MeshLab. 
|
cakarsubasi
force-pushed
the
feature/rsr-hierarchical
branch
from
ce8e305 to
e6cb5c0
Compare
Add working copy of RsR.h/RsR.cpp Working copies of the RsR algorithms for multithreading and collapse/expansion. Nowhere near finished. RSR: Fix collapse overflow bug RsR: refactor to get rid of the convoluted branching for knn_search RsR2.cpp: Use shortest edge based collapse This switches from naive nearest neighbor collapse to naive shortest edge based collapse and I regret not making this change earlier. Not only is this method of collapse much simpler since I have an exact order to perform the collapse and don't have to worry about keeping track of which points are allowed to collapse, since we are selecting based on edges instead of points, I end up selecting both points at the same time. There remains questions however. We select an edge, but there is no obvious way to choose which point is active and which is latent. Should we update the point by averaging it afterwards? If so, then perhaps we should also keep track of point weights so we know how heavy each point is when doing future averaging. If we end up changing point positions, we then have to presumably store the intermediate coordinates. Finally, we need to consider that whatever we do needs to be parallelizable (assuming it is worth parallelizing). If we can do this right and maintain quality after collapses, then the main algorithm will presumably benefit relatively little from parallelization and this part will likely be much more important. RsR2.cpp, RsR2.h: Extended cleanup This performs further dead code elimination and small scale optimizations across these two files. Write separate globals for InvalidNodeID and InvalidEdgeID. (these should be made constexpr later) Parametrize usages of set, unordered_set, and unordered_map. If we ever change from using std containers to a third party one, this makes the change easier to do. The opportunity for performance gains are quite large just from this (>30%). Eliminate dead fields from Vertex and Vertex::Neighbor. Many of these are either write only or their write result is never used. Eliminate the unused API of RSGraph. A copy of it already exists in RsR.h just in case. Eliminate the branch in RSGraph::add_edge. Previously this branch was sometimes taken but ignored (returning an InvalidEdgeID). This was apparently fixed in 925bbda by Andreas. Remove API for remove_duplicate_vertices. Same reason as 1015f691f4c93049162afbe14556a73e7edf3f8b. Remove unnecessary forward declarations in RsR2.cpp. Change naming to be more consistent. Merge knn_search to one function. Simplify nn_search and fix an apparent bug I've created where I did not return a properly sorted version. Write a generic minimum_spanning_tree implementation. This is used two times and it was trivial to factor it out with a functional API. I would not be surprised if there are other parts in the library that also calculate MSTs and we can just later move this so other people can calculate an MST easily. Change the map used in connect_handle not to use a string. remove sets_temp.reserve() built-in sets don't have reserve. Add separate timer for RSR This is just a split of the timer in the original RsR header with some API improvements. There is yet another timer in GEL that seems to be used in a few places (mainly in the demos) that should probably be removed as the built-in chrono in C++ is more than good enough and the other timer has some pretty nasty ifdefs. RsR2.h: Changes Remove <random>. It is not even used. Remove Neighbor related structs. They are moved to Collapse.h. Remove Boolean. It is better it sits next to the only place it is used. Change Vertex::normal_rep. This is another narrowing conversion. Annoyingly, it has two special states (-1 and -2), one of which is a valid state for the NodeID which also inhabits it. Although this is unlikely to cause any real issues, I don't know what to do about it right now. Remove dead RSGraph fields. total_edge_length is never read from inside the code. Maybe it was used for testing at some point. is_euclidean and is_final are better expressed as function arguments, but as it turned out. is_final was completely dead. current_no_edges can be figured out by other means but as it turns out, it is also never read. Remove dead RSGraph function init. It is supposedly called in one place where it was commented out. It might be better to extend AttributeVector API to have a reserve function later. Remove collapse_points API. It is moved to Collapse.h. RsR2.cpp: Changes Reorder arguments in find_common_neighbor. Most functions take RSGraph as the first argument. Remove dead helper functions erase_swap, erase_if_unstable, safe_div. Move knn_search, build_kd_tree_of_indices, calculate_neighbors, collapse_points to Collapse.h Make more stuff const. Move Boolean wrapper inside correct_normal_orientation. Eliminate multiple dead std::vector instances. Make is_intersecting and more compact. Remove commented out check_face_overlap examples. Give explore is_euclidean as an argument instead of a field in G. Make check_branch_validity more compact. In addition, factor out is_final field in RSGraph, it is only ever called with true. As it turns out, the entirety of check_and_force is dead code. Remove check_and_force. Use the constant vert_count overload of build_manifold inside component_to_manifold. RSR2.h: Changes Remove unnecessary <ranges> Move successor and predecessor functions inside RSGraph. Also, see issue janba#105. RsR2.cpp: Changes Move successor and predecessor to RsR2.h. Also update usages so they use the member function in RSGraph. Use a type definition for ThreadPool so I can easily change the desired implementation for each usage. It might a better idea to have the executors be propagated down rather than re-created but with the current execution times, I don't really see anything besides ImmediatePool being worth using. register_face: make the function more compact. connect_handle: style improvements. triangulate: eliminate dead variables RsR2.h: add crash assertions RsR2.cpp: Add crash assertions Move unnecessary type definitions to RsR2.cpp, remove using namespace declarations, add some type definitions inside detail. Move type definitions inside RsR2.cpp Simplify cal_radians_3d Fix rest of typedefinitions, style improvements Add finer timer to component_to_manifold Edge arrays use a single vector Normal estimation does not create intermediate vectors register_face style improvements Style improvements RsR2: make a few things constexpr RsR2: Minor fixes RsR2: Changes Move neighbor calculation and cross connection filtering out of init_graph. init_graph now returns the graph it initializes directly. Make the inner loop a little more sensible. Rename generic mst builder to make_mst Factor out RSGraph::exp_genus to be a function argument Move neighbor calculation out of split_components. Remove completely pointless inner loop in graph construction. Make usages a little more clear in point_cloud_to_mesh RsR2.h apply changes in previous commit RsR2.h reshuffle things around a bit RsR2.cpp: Changes Move most using declarations to the top. Use a common comment style. Eliminate one unnecessary allocation in nn_search Factor out max_length and pre_max_length in init_graph to a single vector. Eliminate two dead arguments to find_shortest_path weighted_smooth is now alloc free Replace call to neighbors with neighbors_lazy RsR2.cpp: remove triangle_mean_normal remove constexpr from normalize_normals revert vertex constructor change (Clang) RsR2.h Use phmap associative containers Add access assertions to tree_id Update point_cloud_collapse_reexpand signature to have CollapseOpts RsR2.cpp Use lazy neighbor iterators Add more input sanitization Update point_cloud_collapse_reexpand signature RsR2.cpp: I want to split the data and behavior inside the Collapse header but I can't do that right now, so we will insert some temporary changes here and fix up the API of collapse_points and reexpand_points later RsR2.h: Switch to SparseGraph This actually causes a performance regression but I actually want to better encapsulate the two graphs inside RsR RsR2.cpp: Switch to SparseGraph This actually causes a performance regression but I actually want to better encapsulate the two graphs inside RsR RsR2: Many improvements Make SimpGraph no longer derived. (I'm not so sure about this change and might revert it since we have to expose the inner part anyway). Instead of using AttribVec, use std::vector in SimpGraph and RSGraph. This results in a major performance improvement. Revert SimpGraph and RSGraph to use AMGraph. The SparseGraph idea wasn't bad but we have too many edges for it to be anywhere near performant. Move get_neighbor_info and maintain_edge_loop inside RSGraph. That's where they make sense. Convert a few pairs to dedicated structs to avoid potential memcpy fails. Convert a lot of values to references to avoid copying. As it turns out this was one of our biggest performance losses because each Vec3 is 24 bytes and certain functions are copying quite a few of them. In this case, inlining becomes undesirable because the indirect memory access will nearly always beat copying so much data to the stack over and over. Add some experimental multithreading to edge connection. RsR2.cpp: Merge four allocations into one in connect_handle, move a function to RangeTools Revert constructor deletions in RSRTimer so I can more easily drop this into the old RsR.cpp Remove failed reserve attempt, make AMGraph privately inherited in RSGraph Change RsR2.cpp to use the intended collapse API RsR2: Revert several changes to fix some regressions The main things are, the imp_node vector is not populated correctly in connect_handle and cross connection filtering early results in the pre_max_length being incorrect. These do not explain the entirety of the regressions. Remove incorrect constexpr RsR.cpp Update the interface to handle normal and no normal situations correctly RsR2: take a full option struct for reexpansion options RsR2: eliminate unnecessary NodeID from Vertex struct RsR2: simplify geometry check, add debug functions Oops, forgot to clear neighbors Even more fixes to RSR2
This adds some of the collapse-reexpansion functionality as a separate header. Likely not hugely desirable, but RsR2.cpp has over 2000 lines and CLion does not like analyzing it. If I can get rid of some of the unwanted repetition, I will consider merging this back to RsR2.h/RsR2.cpp. Collapse: add numbers import so we can pull pi Collapse.h: Add thresholding for angles and valencies to reexpansion As Andreas pointed out, it is most likely better to use thresholds to limit triangles with more extreme minimum angles and vertices with too few way too many neighbors. When set very conservatively, this seems to result in a slight improvement in reconstruction quality. Collapse.h: Changes Move Collapse types and functions into RSR. They will only be used there. Move collapse_neighbors from RsR2.cpp to here. Technically, this means we need to move a few things that are not collapse related (mainly the neighbor calculations) but this means that all Collapse-related code (except for the API) is in Collapse.h. I will likely further move stuff into its own compilation unit. Collapse.h: Make Clang shut up It seems that Util::Range returns a i64 regardless which is probably not what we want. Since I will most likely remove that in exchange for iota_view (despite the apparent downsides) This will likely not be a problem later. Use Vec3 type definition Collapse: implement fundamentals for QEM Collapse: formatting Collapse.h Use QEMs for collapse and reexpansion Collapse.h: a lot of changes Add CollapseOpts to pass some parameters more easily. Add zip_view_t to make easy pairs. TODO: move this elsewhere. Add zip helper template for zip_view_t Add cartesian_product helper template. Add normal lerp functions. Fix a fairly nasty bug in QuadraticCollapseGraph that resulted in horrible collapses. Experimental shared_neighbors normalization in QEM (Currently seems to make things worse). Some temporary changes to Collapse while I figure out how to properly pass this data around. Add a struct that just contains the collapse information (preferred thing to pass to the second function). Add a helper function to convert the original collapse struct to this. Create another helper function to create artificial collapses. optimize_dihedral might have some issues? I need to check we are actually getting a proper value. Use a split struct to be more explicit. Use a triangle struct instead of trying to manage triangles manually. I don't care that this is 72 bytes, the previous method was a complete nightmare. find_edge_pair tries to find the actual one_ring and two_ring dihedrals now. I also added a bunch of debug printing that I will most likely remove but I want to leave it in the commit history just in case. Add decimate and decimate_reexpand functions to investigate results based on a collapse directly from the Manifold instead of relying on the more noisy results from the RSR. Split Collapse.h to Collapse.cpp Move even more stuff to Collapse.cpp Save the futile alternative edge flip based reconstruction method in Collapse.cpp I will most likely just remove this immediately but I want to have a reference here just in case I want to come back to it later Collapse.cpp: I finally figured out how to get OK performance, so save here so I can make it actually fast Collapse.cpp Perform many triangle optimizations to reduce amount of work during a reexpansion Collapse: remove the fat Collapse class Collapse: early return for 0 max iterations Collapse.h Also return indices for the point cloud Collapse: Replace MutablePriorityQueue with a simpler BTree based design that is much faster Major changes to Collapse Add debug options for easier debugging Bring back the old somewhat inefficient QuadraticCollapseGraph based on the MutablePriorityQueue. The simpler BTree based version should still function well hypothetically but until I figure things out, both can live here. Clamp the minimum error for QEMs to a numerically stable value, so at very low QEM values, distance will dominate. Angle optimization multiplies by the edge length that the angle is facing. This is somewhat problematic but it avoids the problem that the angle scoring being unaffected by the edge length while dihedral angles were. So I no longer have to worry about applying an awkward normalization factor when balancing them out. Add one_ring_max_angle for debugging by figuring out if we are creating acute angles. I used 1 for debug thresholding but it seems like there are cases where such acute angles are not unreasonable, although we will have to discuss this during a meeting. Take into account angles during split selection. Return the actual maximum dihedral angle rather than the edge adjusted maximum dihedral angle. Store two sets of potential splits. One based on just our scoring, and the other that avoids any acute (>90 deg) angles. Pick the non-acute angle if possible. Refactor the edge crossing check to shoot_ray_and_maybe_flip_edge (better name pending). What this now does is that it checks if we are crossing or getting close to cross a plate around a particular edge. I might want to set the "about to cross" threshold as a potential out parameter. Make collapse and reexpansion work. There, I said it. Final behavorial changes to Collapse and Reexpansion
Add RsR tests Move KDTree builder to test file RSR: Fix collapse overflow bug RSR_test.cpp: Remove duplicates testing Since I have figured out that duplicates are checked in multiple places in the original reconstruction, explicitly removing duplicates have become unnecessary. In addition, I didn't like this oracle testing method either. Collapse API is in HMesh::RSR now. Better reconstruct_debug Add RsR tests to CMakeLists.txt RsR_test.cpp: Changes Remove unused create_rectangular_manifold Minor fixes. Pass old reconstruct_single the pre-generated normals. Collapse: implement fundamentals for QEM RsR_test.cpp Comment outdated tests, adapt existing tests for the new signatures RsR_test.cpp Add rectangular tests and a bunch of other nonsense RsR_test: Split include_directories again RsR_test: Remove unnecessary RangeTools and SparseGraph tests Update RsR_test to reflect the latest changes Make RsR_test compile again
Fix reexpansion soundness issue
…old mesh reconstruction.
Renamed stuff for better clarity. Moved all nonessential parts of RSRE out of the header. Added documentation for the RSROpts struct
Create NeighborUtil and factor commonly used functions in RSR and HRSR to there. Move private types of HRSR to the cpp file. Cleanup include files.
cakarsubasi
force-pushed
the
feature/rsr-hierarchical
branch
from
85df237 to
1720bbf
Compare
cakarsubasi
force-pushed
the
feature/rsr-hierarchical
branch
from
8f37b75 to
f543e93
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pre-merge checklist: