Dynamic loading of different mesh formats #66

amock · 2024-11-15T19:08:43Z

What I did:

One can load meshes from common mesh file formats now. It is also possible to load a specific mesh from a whole scene as navigation mesh. One problem remained: Loading the ".dae"s of the mesh nav tutorials gave wrong results. I don't know why. Internally it is divided into an input file and working file. The input file is an arbitrary mesh format (that can be loaded by assimp or HDF5) the working file is always a HDF5. I added a service which can be used to save the current state to the working file.
The layers are now stored in the HDF5 file by their name. This allows us to integrate many layers of the same type.
I quickly tested some of the tutorials and everything worked out fine. So it should be downwards compatible.
Some bug fixes.

- removed some layer dependent variables - removed server connection. It can be readded at a later point. But right now it is old code and we don't know how it works. So for now I think it's better to remove it.

Cakem1x

Nice additions to the map! I also like that you removed the service code.
Being able to load more kinds of meshes sounds very useful.

I added a few comments, the logic looks good.
I have some remarks that hopefully increase readability/maintainability, please check them out.

Cakem1x · 2024-11-19T08:47:50Z

mesh_layers/src/border_layer.cpp

    rcl_interfaces::msg::FloatingPointRange range;
    range.from_value = 0.0;
    range.to_value = 1.0;
    descriptor.floating_point_range.push_back(range);
-    config_.factor = node_->declare_parameter(mesh_map::MeshMap::MESH_MAP_NAMESPACE + "." + layer_name_ + ".factor", config_.factor);
+    config_.factor = node_->declare_parameter(mesh_map::MeshMap::MESH_MAP_NAMESPACE + "." + layer_name_ + ".factor", config_.factor, descriptor);


Whoops, was the descriptor never used when declaring the parameter? 🫢
Nice fix.

Yeah, but at least it was described for anyone reading the code 😅

Cakem1x · 2024-11-19T08:51:25Z

mesh_map/include/mesh_map/mesh_map.h


  //! dynamic params callback handle
  rclcpp::node_interfaces::OnSetParametersCallbackHandle::SharedPtr config_callback;

+  rclcpp::Service<std_srvs::srv::Empty>::SharedPtr save_service;


I think it would be nice to get a response on whether saving worked out.

# empty request? string path_to_map # or maybe allow defining a path here? --- bool was_successful string failure_reason # empty string if was_successful is true. Otherwise, e.g. "cannot open file '/path/to/map'"

Still, leaving it like it is now is a nice upgrade on the previous version. 👍

Yes, I would like this too. Perhaps something for the near future. Is there a std_srvs service for that already?

Yes, indeed: std_srvs/Trigger

I changed the service from Empty to Trigger

Cakem1x · 2024-11-19T08:59:40Z

mesh_layers/include/mesh_layers/border_layer.h

@@ -141,6 +141,9 @@ class BorderLayer : public mesh_map::AbstractLayer
    double border_cost = 1.0;
    double factor = 1.0;
  } config_;
+
+  // put this to base class?
+  std::string name;


codestyle

Suggested change

std::string name;

std::string name_;

Wait, is this even used? I think you're only using the layer_name_ attribute from the base class in the cpp file.

Looks like this is a leftover from experimenting.

Yes, it's basically how it is written in the comments. "Put this to base class" then I jumped to the base class and saw, there is a variable already. So "name" can be removed safely.

Cakem1x · 2024-11-19T09:08:38Z

mesh_map/include/mesh_map/mesh_map.h

+  //! mesh file 
  std::string mesh_file;
+
+  //! mesh part
  std::string mesh_part;
+
+  //! mesh working file
+  std::string mesh_working_file;
+
+  //! mesh working part
+  std::string mesh_working_part;


The comments are superfluous, since they only consist of the variable's name. Leaving them in place does not add any clarity and they might become out-of-date when the code changes, leading to confusion.
Either remove them or replace them by an explanation, if necessary.

Also, you could add a path or file_path to the variable name, if it is a path. I think e.g. mesh_file is probably not the actual file (or pointer to some data in that file), but just the filesystem path. Since its type is std::string! :) But that might just be my personal preference. 🤷

What working means could use an explanation.
Why are there working and non-working versions of file&part?

Maybe salvage a few words from your PR description:
Internally it is divided into an input file and working file. The input file is an arbitrary mesh format (that can be loaded by assimp or HDF5) the working file is always a HDF5. I added a service which can be used to save the current state to the working file.

Or maybe find better names, e.g. call them mesh_input_file/mesh_input_part and mesh_working_file/mesh_working_part.

The comments are superfluous, since they only consist of the variable's name. Leaving them in place does not add any clarity and they might become out-of-date when the code changes, leading to confusion. Either remove them or replace them by an explanation, if necessary.

Agree. I was just adapting to the comment style that was already there 😁 There could be a complete PR about cleaning up all of the comments plus all the error messages.

Also, you could add a path or file_path to the variable name, if it is a path. I think e.g. mesh_file is probably not the actual file (or pointer to some data in that file), but just the filesystem path. Since its type is std::string! :) But that might just be my personal preference. 🤷

Agree, I was again just adapting the style. However, I would move that to a seperate "better style" PR.

What working means could use an explanation. Why are there working and non-working versions of file&part?

Maybe salvage a few words from your PR description: Internally it is divided into an input file and working file. The input file is an arbitrary mesh format (that can be loaded by assimp or HDF5) the working file is always a HDF5. I added a service which can be used to save the current state to the working file.

Or maybe find better names, e.g. call them mesh_input_file/mesh_input_part and mesh_working_file/mesh_working_part.

Agree. I will push a commit with some docs soon.

Great, thanks!

mesh_map/include/mesh_map/mesh_map.h

Cakem1x · 2024-11-19T09:31:35Z

mesh_map/src/mesh_map.cpp

+      // RCLCPP_INFO_STREAM(node->get_logger(), "Writing '" << layer_name << "' to file.");
+      // layer_plugin->writeLayer();
+      // RCLCPP_INFO_STREAM(node->get_logger(), "Finished writing '" << layer_name << "' to file.");


Please delete these lines, if they are not necessary anymore.

Or, if you think they might become useful later, maybe make them DEBUG logs. This will ensure they get considered in refactorings and won't become weird dead code (comments) in the long run.

Cakem1x · 2024-11-19T09:33:42Z

mesh_map/src/mesh_map.cpp

+    // TODO how to declare param for each plugin? 
+    // Needs to be done after plugins are loaded, which happens when the map gets loaded. Who calls readMap() and when?


Is this outdated?

Each plugin needs to define its own parameters. They do that in the initialize() method, but I see that you already found that method.

outdated. removed

mesh_map/src/util.cpp

Cakem1x · 2024-11-19T09:43:01Z

mesh_map/src/util.cpp

 namespace mesh_map
 {
+
+// again this function


What do you mean by that? Did you write / see this function someplace else already?

This was just a joke. Because in 2024 I still have to write a split_by_delimiter function for a std::string. I removed it

Cakem1x · 2024-11-19T09:52:51Z

mesh_map/src/mesh_map.cpp

+    std::shared_ptr<std_srvs::srv::Empty::Response>      response)
+  {
+    saveLayers();
+  });
 }

 bool MeshMap::readMap()


Reading the code that loads the mesh map was a bit confusing with all the different cases and different ways the working/non-working variables can be filled (via user or automatically).
Is there a way to simplify the logic?

Perhaps there is a way. But I haven't found it yet :D

Co-authored-by: Matthias Holoch <[email protected]>

Cakem1x

thanks for reiterating! 👍

Cakem1x · 2024-11-19T13:53:56Z

mesh_map/include/mesh_map/mesh_map.h

@@ -434,16 +441,22 @@ class MeshMap
  //! global frame / coordinate system id
  std::string global_frame;

-  //! mesh file 
+  //! Filename of the input mesh. Can be any format the assimp library supports to load. 
+  //! https://github.com/assimp/assimp/blob/master/doc/Fileformats.md


amock · 2024-11-19T23:45:41Z

I tested the three maps from the mesh nav tutorials and everything went fine.

Cakem1x · 2024-11-20T09:33:01Z

I just ran a few successful tests with your branch and our tutorials.

amock added 8 commits November 14, 2024 23:17

Cleanup of mesh map:

6367eb2

- removed some layer dependent variables - removed server connection. It can be readded at a later point. But right now it is old code and we don't know how it works. So for now I think it's better to remove it.

added template for generic mesh file loading

edd9cb2

Added conversion from assimp scene to lvr meshbuffer

6324829

added todos

f1ed54a

loaded geometry from a common mesh file format

ea31a4d

loading PLY works. DAE cannot be loaded still don't know why.

544bbba

added service to save the changed layers

fa39eb4

layer are pointing to same named hdf5 datasets

ec6721f

amock requested a review from Cakem1x November 15, 2024 19:08

Cakem1x requested changes Nov 19, 2024

View reviewed changes

amock and others added 7 commits November 19, 2024 11:21

Update mesh_map/include/mesh_map/mesh_map.h

1c9a67e

Co-authored-by: Matthias Holoch <[email protected]>

Update mesh_map/src/mesh_map.cpp

07ca274

Co-authored-by: Matthias Holoch <[email protected]>

Update mesh_map/src/mesh_map.cpp

acb2c0f

Co-authored-by: Matthias Holoch <[email protected]>

cleanup

c983453

Update mesh_map/src/util.cpp

0787ac1

Co-authored-by: Matthias Holoch <[email protected]>

removed name member variable since it is not used anymore

c087a33

added docs to saveLayers

2b95a06

Cakem1x self-requested a review November 19, 2024 13:52

Cakem1x approved these changes Nov 19, 2024

View reviewed changes

amock added 2 commits November 20, 2024 00:10

added std_srvs Trigger instead of Empty

8524819

if only a h5 is provided the same file is used as input and working file

ef42e0a

amock requested a review from Cakem1x November 19, 2024 23:46

amock merged commit fedea89 into naturerobots:humble Nov 20, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic loading of different mesh formats #66

Dynamic loading of different mesh formats #66

amock commented Nov 15, 2024 •

edited

Loading

Cakem1x left a comment

Cakem1x Nov 19, 2024

amock Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024

Cakem1x Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024 •

edited

Loading

Cakem1x Nov 19, 2024

Cakem1x Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024

amock Nov 19, 2024

amock Nov 19, 2024

Cakem1x Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024

Cakem1x Nov 19, 2024

Cakem1x Nov 19, 2024

amock Nov 19, 2024

Cakem1x left a comment

Cakem1x Nov 19, 2024

amock commented Nov 19, 2024 •

edited

Loading

Cakem1x commented Nov 20, 2024

		// TODO how to declare param for each plugin?
		// Needs to be done after plugins are loaded, which happens when the map gets loaded. Who calls readMap() and when?

Dynamic loading of different mesh formats #66

Dynamic loading of different mesh formats #66

Conversation

amock commented Nov 15, 2024 • edited Loading

Cakem1x left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amock Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cakem1x left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amock commented Nov 19, 2024 • edited Loading

Cakem1x commented Nov 20, 2024

amock commented Nov 15, 2024 •

edited

Loading

amock Nov 19, 2024 •

edited

Loading

amock commented Nov 19, 2024 •

edited

Loading