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

Fix collision checking in VisibilityConstraint #1986

Conversation

schornakj
Copy link
Contributor

Description

Previously, planning with the VisibilityConstraint could cause segfaults because modifying collision objects in the collision environment is not thread-safe. The planning threads would interfere with each other by trying to add and remove the visibility cone collision object out of sync.

This PR fixes that by creating a local collision environment within VisibilityConstraint::decide.

I also modified how VisibilityConstraints are evaluated so that if a negative or zero target_radius value is used but valid max_range_angle and max_view_angle are provided, then the visibility cone check is skipped and the range and view angle checks are performed as usual. Previously setting an invalid target_radius value would cause the entire constraint evaluation to be skipped.

While I was at it I renamed some of the variables used for transforms to be more verbose and easier to follow.

Note: I'm keeping this as a draft for now since I'm still testing, but I'd be happy to get feedback on what I have so far.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage: 70.00% and project coverage change: +0.01 🎉

Comparison is base (127d483) 50.90% compared to head (b2896e7) 50.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1986      +/-   ##
==========================================
+ Coverage   50.90%   50.91%   +0.01%     
==========================================
  Files         391      391              
  Lines       32128    32115      -13     
==========================================
- Hits        16353    16349       -4     
+ Misses      15775    15766       -9     
Impacted Files Coverage Δ
...oveit/kinematic_constraints/kinematic_constraint.h 93.75% <ø> (ø)
...ry_processing/time_optimal_trajectory_generation.h 100.00% <ø> (ø)
...cessing/src/time_optimal_trajectory_generation.cpp 86.51% <ø> (-0.04%) ⬇️
...kinematic_constraints/src/kinematic_constraint.cpp 74.65% <70.00%> (+0.45%) ⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@schornakj schornakj force-pushed the pr-visibility-constraint-collision-check-rolling branch from 57ccae8 to 5592273 Compare March 11, 2023 00:36
@schornakj schornakj force-pushed the pr-visibility-constraint-collision-check-rolling branch from 5592273 to b2896e7 Compare April 7, 2023 19:58
@schornakj schornakj marked this pull request as ready for review April 7, 2023 19:58
sjahr added a commit to PickNikRobotics/moveit2 that referenced this pull request Apr 27, 2023
…onstraint

commit 03dcec06b2ecbb16182c3c4519268b7cd0b8d8c3
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 19:16:19 2023 -0500

    use same transform math for fixed and mobile frames

commit e7b578d82a7237a0e05672edcb07d6989de15962
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 17:53:59 2023 -0500

    reduce redundant transform calculations

commit 3e1896ea557be6feab5d9ae7c9a27c9104b789f9
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:07:37 2023 -0500

    clang-format fixes

commit 2613f9cd1033b4fad71f8d5156508adb303d299f
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:05:36 2023 -0500

    delete commented-out code

commit 648598d72036b0cdc477193ad692e0d670730e5a
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 16:50:32 2023 -0500

    disable visibility cone check if target radius is zero

commit 718631f98f12052a22a9a8cc1fc842ef99dff911
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Wed Mar 1 20:10:35 2023 -0500

    use local collision environment to check constraint
sjahr pushed a commit to PickNikRobotics/moveit2 that referenced this pull request Apr 27, 2023
…onstraint

commit 03dcec06b2ecbb16182c3c4519268b7cd0b8d8c3
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 19:16:19 2023 -0500

    use same transform math for fixed and mobile frames

commit e7b578d82a7237a0e05672edcb07d6989de15962
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 17:53:59 2023 -0500

    reduce redundant transform calculations

commit 3e1896ea557be6feab5d9ae7c9a27c9104b789f9
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:07:37 2023 -0500

    clang-format fixes

commit 2613f9cd1033b4fad71f8d5156508adb303d299f
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:05:36 2023 -0500

    delete commented-out code

commit 648598d72036b0cdc477193ad692e0d670730e5a
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 16:50:32 2023 -0500

    disable visibility cone check if target radius is zero

commit 718631f98f12052a22a9a8cc1fc842ef99dff911
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Wed Mar 1 20:10:35 2023 -0500

    use local collision environment to check constraint
sjahr pushed a commit to PickNikRobotics/moveit2 that referenced this pull request Apr 27, 2023
…onstraint

commit 03dcec06b2ecbb16182c3c4519268b7cd0b8d8c3
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 19:16:19 2023 -0500

    use same transform math for fixed and mobile frames

commit e7b578d82a7237a0e05672edcb07d6989de15962
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 17:53:59 2023 -0500

    reduce redundant transform calculations

commit 3e1896ea557be6feab5d9ae7c9a27c9104b789f9
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:07:37 2023 -0500

    clang-format fixes

commit 2613f9cd1033b4fad71f8d5156508adb303d299f
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:05:36 2023 -0500

    delete commented-out code

commit 648598d72036b0cdc477193ad692e0d670730e5a
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 16:50:32 2023 -0500

    disable visibility cone check if target radius is zero

commit 718631f98f12052a22a9a8cc1fc842ef99dff911
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Wed Mar 1 20:10:35 2023 -0500

    use local collision environment to check constraint
sjahr pushed a commit to PickNikRobotics/moveit2 that referenced this pull request May 8, 2023
…onstraint

commit 03dcec06b2ecbb16182c3c4519268b7cd0b8d8c3
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 19:16:19 2023 -0500

    use same transform math for fixed and mobile frames

commit e7b578d82a7237a0e05672edcb07d6989de15962
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Fri Mar 10 17:53:59 2023 -0500

    reduce redundant transform calculations

commit 3e1896ea557be6feab5d9ae7c9a27c9104b789f9
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:07:37 2023 -0500

    clang-format fixes

commit 2613f9cd1033b4fad71f8d5156508adb303d299f
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 20:05:36 2023 -0500

    delete commented-out code

commit 648598d72036b0cdc477193ad692e0d670730e5a
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Thu Mar 2 16:50:32 2023 -0500

    disable visibility cone check if target radius is zero

commit 718631f98f12052a22a9a8cc1fc842ef99dff911
Author: Joe Schornak <joe.schornak@gmail.com>
Date:   Wed Mar 1 20:10:35 2023 -0500

    use local collision environment to check constraint
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

I think we should get this fixed merged for now to get rid of the segfault. @schornakj since you are using this, do you think you could evaluate the performance impact and decide if we need to address this further?

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

LGTM

@henningkayser henningkayser merged commit 7c6da55 into moveit:main May 9, 2023
sjahr pushed a commit to PickNikRobotics/moveit2 that referenced this pull request May 12, 2023
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.

None yet

3 participants