search

ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.61k stars 1.3k forks source link

Remove all the mesh files and stl files from nav2_system_tests #1337

Closed mlherd closed 4 years ago

mlherd commented 5 years ago

There are tb3 robot models and description files in the system tests. For the system test that we have, we don't need to have any visuals in these files. Simple shapes such as boxes can be used in burger.urdf and waffle.urdf and no visuals in gazebo models.

SteveMacenski commented 5 years ago

Is there a reason we don’t have a dependency of system tests on the TB3 repos and just call them from their packages?

The system tests can have dependencies on specific artifacts and I think that’s totally fine.

mlherd commented 5 years ago

Because, in nav2_system_tests, currently we duplicate their files in our repo. The size of Nav2 is about 82MB and about 62 MB of it is these files. In addition to that, why should we have a dependency when we can easily avoid that dependency?

SteveMacenski commented 5 years ago

Because, in nav2_system_tests, currently we duplicate their files in our repo

Exactly. If its just a dependency, its not in this repo to duplicate.

why should we have a dependency when we can easily avoid that dependency?

Duplication isn't really removing the dependency, its just adding more maintenance requirements for us. Even by adding the simplified versions to system tests, its now more maintenance that there would be if we just used the turtlebot packages directly.

In my opinion, system tests should not ship with the metapackage navigation2 but can still be installed separately to test the install. That way you don't make people rely on TB3 repos for a production build

crdelsey commented 5 years ago

Is there a reason we don’t have a dependency of system tests on the TB3 repos and just call them from their packages?

We'd need to add the necessary TB3 repos to our ros2_dependencies.repos file in order to build master. Now that the nav2_system_test package is built by default, anyone building our repo from source would need to have those packages available as well, even if they intend to work with another robot.

Also, Robotis has our stuff as a dependency of theirs. Adding their repos as dependencies of ours is technically OK at a per-package level, but it is circular at a github repo level. It wouldn't be possible to build their workspace as an overlay of ours. They would have to combine nav2 and their stuff in one big workspace.

I think all of this is solveable with sufficient communication, but it just seems easier to me at this point to copy the minimal essential files to our system test folder, and not add the TB3 dependencies.

SteveMacenski commented 5 years ago

Fair enough. We can also just change to another default robot model. Either way I don't have a strong opinion, just trying to make sure we're duplicating the absolute minimum possible

SteveMacenski commented 4 years ago

@mlherd do you think from the introduction of the files into nav2_bringup for laser/depth camera we can remove all the STLs/URDFs from nav2 system tests and use that instead?

SteveMacenski commented 4 years ago

@mlherd is this something you're still open to doing? It would save me a ton of time to work on fixing other issues if I didn't have to learn how we deal with meshes for the testing setup right now

mlherd commented 4 years ago

@SteveMacensk I remember #1249 was the first step for this issue. I can work on this issue sometime next week.

ROBOTIS-Will commented 4 years ago

@mlherd I can help with simplifying TurtleBot3 mesh files. Not sure where the DAE files first came from, but I think using a different 3D file only in Nav2 simulation wouldn't be critical. Combining NAV2 into a TurtleBot3 WS can be reviewed later if it is not urgent.

mlherd commented 4 years ago

@ROBOTIS-Will I would really appreciate your help! I have been able to work on this yet. Please feel free to take the lead.

In my opinion, for Nav2 bring up it is ok to use 3D visuals, but for testing purposes, no 3D visuals needed.

I already created a model file with no 3D files. https://github.com/ros-planning/navigation2/tree/master/nav2_system_tests/models/turtlebot3_waffle_depth_camera

Also, I created a urdf file with no 3D files. I used boxes and cylinders instead.

https://github.com/mlherd/navigation2/blob/2fd3f150169aa230b7f7f939da2912bd43a3b5ae/nav2_system_tests/urdf/turtlebot3_waffle.urdf

My plan was to use these files in the system tests instead and get rid of all the 3d files.

ROBOTIS-Will commented 4 years ago

@mlherd Agreed. I don't think complicated modeling files wouldn't be necessary for simple simulation. I've been working on simplified version of DAE files that will reduce the size of current files up to 90%. (waffle_base will be about 3.2MB) But definitely boxes and cylinders would be a good option to have!

SteveMacenski commented 4 years ago

Keeping for now until TB3 repos are actually released and supported with configs.