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

Do not override empty URDF link collision geometry #1952

Merged
merged 2 commits into from
Mar 13, 2020

Conversation

Dale-Koenig
Copy link
Contributor

@Dale-Koenig Dale-Koenig commented Mar 13, 2020

Partially addressing #130. Add a warning so that more drastic action can be taken when this issue is revisited in the future, and to ensure that the user is aware that the collision geometry has been altered.

Fixes #130 and fixes #1907. Changes to no longer override collision geometry.

@codecov-io
Copy link

codecov-io commented Mar 13, 2020

Codecov Report

Merging #1952 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1952   +/-   ##
=======================================
  Coverage   49.82%   49.82%           
=======================================
  Files         313      314    +1     
  Lines       24714    24715    +1     
=======================================
+ Hits        12313    12314    +1     
  Misses      12401    12401           
Impacted Files Coverage Δ
moveit_core/robot_model/src/robot_model.cpp 76.64% <100.00%> (-0.08%) ⬇️
...ove_group_interface/src/wrap_python_move_group.cpp 43.03% <0.00%> (ø)
...ls/include/moveit/py_bindings_tools/gil_releaser.h 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5327e41...6c95dd4. Read the comment docs.

@v4hn
Copy link
Contributor

v4hn commented Mar 13, 2020

Thank you for the patch. The maintainer meeting actually agreed to a more drastic patch in the master branch, but no one acted on it yet.

Could you change your patch to keep the warning, but not add the collision geometry?

The reasoning was as follows:

  • If we want to change the standard, we should do this in the master branch directly. This is what the branch is for.
  • If we add the warning and anyone really relies on the old behavior, they will just have to copy their geometry to the collision geometry too
  • If people want the new behavior, they will have to ignore the warning for some transition period.

Also, please add a helpful entry to the MIGRATION.md?

@Dale-Koenig Dale-Koenig changed the title Add warning message when overriding collision geometry Do not override empty URDF link collision geometry Mar 13, 2020
Comment on lines 1024 to 1027
ROS_WARN_STREAM_NAMED(LOGNAME, "Link " << urdf_link->name << ": "
"has visual geometry but no collision geometry. "
"Collision geometry will be left empty. "
"Fix your URDF file by explicitly specifying collision geometry.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually propose to set this warning to a different logger name, so that affected users can set this logger to ERROR if they want to ignore the specific warning.
How about LOGNAME+".empty_collision_geometry"?

Suggested change
ROS_WARN_STREAM_NAMED(LOGNAME, "Link " << urdf_link->name << ": "
"has visual geometry but no collision geometry. "
"Collision geometry will be left empty. "
"Fix your URDF file by explicitly specifying collision geometry.");
ROS_WARN_STREAM_NAMED(LOGNAME+".empty_collision_geometry", "Link " << urdf_link->name << ": "
"has visual geometry but no collision geometry. "
"Collision geometry will be left empty. "
"If you require collision checking for the link, explicitly set the collision geometry in the URDF."
"Otherwise, consider disabling this logger name via rosconsole.");

@v4hn v4hn added the awaits 2nd review one maintainer approved this request label Mar 13, 2020
- use more specific variable name
- avoid turning off clang-format
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I added some minor cleanup. Looks good. Thanks!

@v4hn v4hn merged commit 97e3e9b into moveit:master Mar 13, 2020
@Dale-Koenig Dale-Koenig deleted the warn_on_override_collision branch March 15, 2020 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits 2nd review one maintainer approved this request
Projects
None yet
4 participants