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

Self-collision barrier jacobian fixes #115

Merged

Conversation

domrachev03
Copy link
Contributor

This PR addresses two issues I've encountered while working with self-collision barriers:

  • It raised a dimension error when applied to floating base robots
  • It raised an error, when there were more than two floating objects in the model (f.e. when some external obstacles appear, which are not a part of the robot)

Both problems were related to the jacobian computation, i.e. SelfCollisionBarrier.compute_jacobian() method:

  • The first problem was a trivial typo in Jacobian shape: the second dimension was robot.nq instead of robot.nv.
  • The second problem was a little bit more tricky: as far as I understood, floating bodies have a parent joint (i.e. root joint), but they do not have a parent frame. Previously, the implementation of the barrier referred to the parent frame, which resulted in the error for floating bodies. The new implementation refers to joints instead.

I have checked the new implementation in some of my side projects, but I could port the examples there, just for completeness. Alternatively, we might introduce new tests that check that behaviour, if you wish.

Thanks to @simeon-ned and @RumblingTurtle for their support in finding problems and finding solutions to the issues.

@coveralls
Copy link

coveralls commented Sep 21, 2024

Pull Request Test Coverage Report for Build 10972373437

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.088%

Totals Coverage Status
Change from base Build 10580819568: 0.0%
Covered Lines: 1956
Relevant Lines: 1974

💛 - Coveralls

@domrachev03
Copy link
Contributor Author

I'm not sure what's wrong with the documentation, I've changed only 3 words in the docstrings, but the pipeline is failing :/

@stephane-caron
Copy link
Owner

LGTM, thank you for the fix and the clear description 👍

No worries about the documentation, it is related to an upstream update of Sphinx. I will fix it separately.

@stephane-caron stephane-caron merged commit 129df31 into stephane-caron:main Sep 23, 2024
8 of 9 checks passed
@stephane-caron
Copy link
Owner

I could port the examples there, just for completeness.

Feel free to add an example that would have triggered the previous issue 👍 A unit test would be even better to catch regressions, but an example is already somewhere to start from if you have one already.

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.

3 participants