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

LVR Update #70

Open
wants to merge 10 commits into
base: humble
Choose a base branch
from
Open

LVR Update #70

wants to merge 10 commits into from

Conversation

amock
Copy link
Collaborator

@amock amock commented Dec 11, 2024

This is a reaction to the upcoming LVR update (3.0.0). It contains two major changes:

Interchangable Half-Edge-Mesh

The map and all plugins only use the BaseMesh from LVR. This means that as long as you implement the BaseMesh interface of LVR, you can use any half-edge mesh implementation you want. The implementations can be selected with a new ROS parameter “hem” (Half-Edge-Mesh). "hem: pmp" or "hem: lvr" ist possible. If no parameter is specified the new pmp mesh is chosen. The old behavior can be restored by setting "hem" to "lvr". The pmp mesh turned out to be much more robust to load any mesh coming from other meshing software than lvr (such as meshlab or Open3D). Internally, I used a smart pointer to handle the BaseMesh because it makes things easier.

Loading/Writing HDF5

The HDF5 structure has changed slightly over time so that the description of the mesh_tools paper is not valid anymore. I changed parts of it back so that it gets more like the papers again. It also makes things easier to handle since vertex attributes are now separated from the actual mesh geometry inside the HDF5.

Cleanup

I have also removed the mesh_client package for which we have no tests and which is currently not used elsewhere.

Testing instructions

This PR works together with:

amock added 5 commits December 5, 2024 17:33
…em' now. 'pmp' and 'lvr' are implemented.

The old half-edge-mesh can be selected by choosing 'lvr'. The new default half-edge-mesh is 'pmp' since it is more robust to different source meshes. The 'pmp' hem worked also to load all available meshes dynamically.

Cleanup: I removed mesh_client package since it was used nowhere.
Copy link
Member

@Cakem1x Cakem1x left a comment

Choose a reason for hiding this comment

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

Please double check some of the logging statements. Other than that, it looks good!

However, CI seems to disagree with some of the change. As soon as CI is fine, I think we can merge.

std::list<std::pair<mesh_map::Vector, lvr2::FaceHandle>> path;

// mesh_map->combineVertexCosts(); // TODO should be outside the planner

RCLCPP_INFO(node_->get_logger(), "start wave front propagation.");
RCLCPP_DEBUG_STREAM(node_->get_logger(), "start wave front propagation.");
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I appreciate using proper log levels! 👍

Comment on lines 654 to 671

RCLCPP_DEBUG_STREAM(node_->get_logger(), "BLA.");

mesh_map_->publishDebugPoint(original_start, mesh_map::color(0, 1, 0), "start_point");
mesh_map_->publishDebugPoint(original_goal, mesh_map::color(0, 0, 1), "goal_point");

RCLCPP_DEBUG_STREAM(node_->get_logger(), "published point.");

mesh_map::Vector start = original_start;
mesh_map::Vector goal = original_goal;

RCLCPP_DEBUG_STREAM(node_->get_logger(), "Find face.");

// Find the containing faces of start and goal
const auto& start_opt = mesh_map_->getContainingFace(start, 0.4);
const auto& goal_opt = mesh_map_->getContainingFace(goal, 0.4);
const lvr2::OptionalFaceHandle start_opt = mesh_map_->getContainingFace(start, 0.4);
const lvr2::OptionalFaceHandle goal_opt = mesh_map_->getContainingFace(goal, 0.4);

RCLCPP_DEBUG_STREAM(node_->get_logger(), "BLA 2.");
Copy link
Member

Choose a reason for hiding this comment

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

clean up logging statements.

Maybe we can remove all of them, not sure if some are still useful. If so, I suggest logging something more than bla bla bla :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I accidentally left those there :) They are all removed now

@@ -727,6 +741,8 @@ uint32_t CVPMeshPlanner::waveFrontPropagation(const mesh_map::Vector& original_s
const auto t_wavefront_start = std::chrono::steady_clock::now();
const auto initialization_duration_ms = std::chrono::duration_cast<std::chrono::milliseconds>(t_wavefront_start - t_initialization_start);


RCLCPP_DEBUG_STREAM(node_->get_logger(), "GO.");
Copy link
Member

Choose a reason for hiding this comment

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

clean up log

Suggested change
RCLCPP_DEBUG_STREAM(node_->get_logger(), "GO.");
RCLCPP_DEBUG_STREAM(node_->get_logger(), "GO.");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's removed now

@amock
Copy link
Collaborator Author

amock commented Dec 23, 2024

Please double check some of the logging statements. Other than that, it looks good!

However, CI seems to disagree with some of the change. As soon as CI is fine, I think we can merge.

All tests are now running without errors 👍

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.

2 participants