Skip to content

Conversation

ntfshard
Copy link
Contributor

🦟 Bug fix

Fixes #2506

Summary

Using the same constants for checking against zero here to significantly reduce cases when system behaves in unexpected way.
Still for a solid solution we should consider a refactoring a whole algorithm.

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.

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>
@ntfshard ntfshard requested a review from mjcarroll as a code owner October 14, 2024 15:11
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Oct 14, 2024
@peci1
Copy link
Contributor

peci1 commented Oct 14, 2024

LGTM.

I guess the constants could even be a bit higher unless you simulate mm-sized robots. The constants you suggest mean that any motion command above 0.000001 m/s or 0.000001 rad/s is considered non-straight motion. I guess no reasonably sized robot would react to a micrometer/sec command. On the other hand, I don't see anything really problematic practically with this constant.

@peci1
Copy link
Contributor

peci1 commented Oct 14, 2024

Could you please post the result of a test where you instruct the robot with e.g. 2e-6 linear and angular velocities?

@ntfshard
Copy link
Contributor Author

ntfshard commented Oct 14, 2024

I guess I can do it tomorrow, it's too late here, sorry. Maybe @iandresolares can provide, if he tested it

And I guess this constant change is not related to any physical properties of a robot, it's just about math properties of this equations, as mentioned in initial msg

@ntfshard
Copy link
Contributor Author

Could you please post the result of a test where you instruct the robot with e.g. 2e-6 linear and angular velocities?

Yes, sure. It just standing still
ubuntu22-screen0.webm

@peci1
Copy link
Contributor

peci1 commented Oct 15, 2024

Great, thanks! Now I'm more sure this PR will not have any bad side effects.

@azeey
Copy link
Contributor

azeey commented Nov 5, 2024

@osrf-jenkins run tests please

@ntfshard
Copy link
Contributor Author

ntfshard commented Nov 25, 2024

Can we have some conclusion about this hotfix? I mean it still affect us and we still need a backport to previous version (gz-sim7) tbh

@codecov
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (gz-sim9@af92e29). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             gz-sim9    #2651   +/-   ##
==========================================
  Coverage           ?   68.78%           
==========================================
  Files              ?      341           
  Lines              ?    33053           
  Branches           ?        0           
==========================================
  Hits               ?    22735           
  Misses             ?    10318           
  Partials           ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@azeey azeey self-requested a review December 9, 2024 20:12
@azeey azeey enabled auto-merge (squash) December 9, 2024 20:15
@azeey
Copy link
Contributor

azeey commented Dec 9, 2024

@ntfshard gz-sim7 (Garden) has reached EOL and we won't be making anymore binary releases of it, so I'm going to backport this only to gz-sim8.

@azeey
Copy link
Contributor

azeey commented Dec 9, 2024

@Mergifyio backport gz-sim8

@mergify
Copy link
Contributor

mergify bot commented Dec 9, 2024

backport gz-sim8

✅ Backports have been created

@azeey azeey merged commit 6f7d21f into gazebosim:gz-sim9 Dec 9, 2024
10 checks passed
mergify bot pushed a commit that referenced this pull request Dec 9, 2024
Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>
(cherry picked from commit 6f7d21f)
azeey pushed a commit that referenced this pull request Dec 9, 2024
Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>
(cherry picked from commit 6f7d21f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏛️ ionic Gazebo Ionic

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

TrackedVehicle system does unexpected movement on low turn commands.

3 participants