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

Allow Scaling Collision Meshes #62

Conversation

amalnanavati
Copy link
Contributor

Description

This PR allows users to scale a collision mesh when adding it into the planning scene. Note that scaling occurs within the mesh's XYZ axes.

This PR also adds trimesh as a dependency. Although it is not a required dependency (it is only necessary when using meshes), I think it is beneficial to add so that all possible dependencies can be installed with a single rosdep call.

Testing

  • Launch the simulated panda arm: ros2 launch panda_moveit_config ex_fake_control.launch.py
  • Add a collision mesh, verify it is the expected scale: ros2 run pymoveit2 ex_collision_mesh.py --ros-args -p position:="[0.5, 0.0, 0.5]" -p quat_xyzw:="[0.0, 0.0, -0.7071, 0.7071]"
  • Re-add the collision mesh with a scaled X, verify it updates as expected in RVIZ: ros2 run pymoveit2 ex_collision_mesh.py --ros-args -p position:="[0.5, 0.0, 0.5]" -p quat_xyzw:="[0.0, 0.0, -0.7071, 0.7071]" -p scale:="[0.5, 1.0, 1.0]"
  • Re-add the collision mesh with a scaled Y, verify it updates as expected in RVIZ: ros2 run pymoveit2 ex_collision_mesh.py --ros-args -p position:="[0.5, 0.0, 0.5]" -p quat_xyzw:="[0.0, 0.0, -0.7071, 0.7071]" -p scale:="[0.5, 2.0, 1.0]"
  • Re-add the collision mesh with a scaled X, verify it updates as expected in RVIZ: ros2 run pymoveit2 ex_collision_mesh.py --ros-args -p position:="[0.5, 0.0, 0.5]" -p quat_xyzw:="[0.0, 0.0, -0.7071, 0.7071]" -p scale:="[0.5, 2.0, 5.0]"

Copy link
Owner

@AndrejOrsula AndrejOrsula left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

I am not super convinced about including trimesh in the mandatory dependencies, though. I think this could have some undesired effects on certain platforms and user setups due to potential conflicts with either system-wide Python packages (trimesh and its dependencies installed via Python package managers) or one of the countless Python environment overlays. In general, I believe it is best to keep Python packages out of the manifest (they might not be handled properly like setup.py or pyproject.toml dependencies). What do you think?

<exec_depend>python3-trimesh-pip</exec_depend>

The user also already gets an explanatory error if they are missing the package when calling the appropriate function.

try:
    import trimesh
except ImportError as err:
    raise ImportError(
        "Python module 'trimesh' not found! Please install it manually in order "
        "to add collision objects into the MoveIt 2 planning scene."
    ) from err

@amalnanavati
Copy link
Contributor Author

I'm fine either way, because if we don't add it to pymoveit2's mandatory dependencies, then I'll just add it to the mandatory dependencies of my package that uses pymoveit2.

In general, I believe it is best to keep Python packages out of the manifest (they might not be handled properly like setup.py or pyproject.toml dependencies).

In my experience, for ROS projects it's easiest to manage all dependencies with rosdep. That way, a single command can do the installation across all packages in the workspace, which makes it easier for new team members to onboard. rosdep certainly has its limitations---like not being able to properly handle version numbers for Python dependencies, and using sudo for pip dependencies---but in my experience the benefits outweigh the drawbacks. In order to use a single rosdep command for all dependencies, all dependencies need to be specified in package.xml.

I haven't used ROS/ROS2 with a virtual environment, and I understand how that could get complicated. It seems like at least one person has been able to get rosdep working with venv.

The user also already gets an explanatory error if they are missing the package when calling the appropriate function.

I agree that this error is very helpful (and more than other open-source libraries do 😄 ).

@amalnanavati
Copy link
Contributor Author

I went ahead and removed trimesh as a required dependency.

@AndrejOrsula AndrejOrsula merged commit 0449402 into AndrejOrsula:devel Apr 26, 2024
2 checks passed
AndrejOrsula pushed a commit that referenced this pull request May 10, 2024
* Allow scaling of collision meshes

* Added trimesh to rosdeps for easier installation

* Ran tests with example

* Remove trimesh as a required dependency
@amalnanavati amalnanavati deleted the amaln/scale_collision_meshes_for_upstream branch May 10, 2024 16:02
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