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.

Sign up for GitHub

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

Jump to bottom

Fix: AckermannSteering Plugin limits the minimum angular velocity based on the minimum linear velocity, preventing steering to the right when set to 0 #2734 #2748

Open
wants to merge 2 commits into
base: gz-sim9
Choose a base branch
from
Open

Fix: AckermannSteering Plugin limits the minimum angular velocity based on the minimum linear velocity, preventing steering to the right when set to 0 #2734 #2748

wants to merge 2 commits into from

Conversation

siksal
Copy link

@siksal siksal commented Jan 30, 2025

🦟 Bug fix

Fixes #2734

Summary

This issue is well described by the person who created it. They also provided the fix on the issue page with test and screenshots. I only corrected a typo in their proposed solution and made this PR (I'm surprised why they didn't make a PR). Kindly check the issue here. I also tested before and after the fix to validate the issue and solution.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@siksal siksal requested a review from mjcarroll as a code owner January 30, 2025 15:49
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Jan 30, 2025
@siksal siksal force-pushed the ackermann-steering branch from 014a1ba to a13e650 Compare January 30, 2025 16:05
@azeey azeey requested a review from bperseghetti January 30, 2025 16:08
ahcorde
ahcorde requested changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

You should document these news parameters in the AckermannSteering.hh file.

This is also a behavioral change, we should keep the original behaviour. if these new values are not defined we can stick with the current code. What do you thinkg ?

@azeey ?

@siksal siksal force-pushed the ackermann-steering branch from adecaa3 to f7395db Compare January 30, 2025 17:25
@siksal
Copy link
Author

siksal commented Jan 30, 2025

You should document these news parameters in the AckermannSteering.hh file.

This is also a behavioral change, we should keep the original behaviour. if these new values are not defined we can stick with the current code. What do you thinkg ?

@azeey ?

I have documented the new parameters.

I agree that we should stick to the current code if the new values are not set. I'm also thinking if the linear vel/acc should be explicit, for example min_linear_velocity instead of min_velocity to distinguish between linear and angular velocities.

@bperseghetti
Copy link
Member

You should document these news parameters in the AckermannSteering.hh file.

This is also a behavioral change, we should keep the original behaviour. if these new values are not defined we can stick with the current code. What do you thinkg ?

@azeey ?

I agree on it being a behavior change, looking at the original issue it appears there was a desire to show a turning angle at zero linear velocity. IMO that doesn't really make sense as I laid out in a response here. The whole point of allowing a "steering only" option was to allow for those that wanted to decouple the steering from the limitations of linear_vel being needed for any angular_vel. Also note that the angular rate limits are updated in UpdateVelocity.

I think the actual bug is not that it "doesn't steer left with zero linear_vel", I think it's actually that it "steers right with zero linear_vel".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde requested changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll is a code owner

@bperseghetti bperseghetti Awaiting requested review from bperseghetti

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
🏛️ ionic Gazebo Ionic
Projects
Core development
Status: In review
Milestone
No milestone
3 participants
@siksal @bperseghetti @ahcorde