Preserve sign of thrust_coefficient

[clydemcqueen]

🦟 Bug fix

Fixes #1399

Summary

Preserve the signs of thrust and thrust_coefficient so that ThrustToAngularVec() is the inverse of AngularVelToThrust(). This provides support for clockwise thrusters.

I modified the ThrusterTest fixture to also preserve the signs of thrust and thrust_coefficient. I added one test. All tests pass.

make codecheck seems to generate a lot of unrelated cppcheck problems, perhaps I have something set up wrong?

./tools/clang_tidy.sh seemed to get stuck on /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/variant -- I'm not sure what is going on there.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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.

[clydemcqueen]

FYI @arjo129 @mabelzhang

[clydemcqueen]

DCO check failed; I just uploaded my key, so it should succeed next time. Closing & reopening.

[arjo129]

Thanks for the changes.

To fix the DCO check out the instructions here:
https://github.com/ignitionrobotics/ign-gazebo/pull/1402/checks?check_run_id=5612527595
You need to git rebase HEAD~2 --signoff and the force push git push --force-with-lease origin sfix_1399.

Regarding codecheck as long as make codecheck returns a zero you can ignore the previous warnings.

A few other knits:

• We need to document the default behavior of the propeller being counterclockwise when positive and clockwise when negative in src/systems/thruster/Thruster.hh
• We need to verify if this behaviour is consistent when <use_ang_vel> is set.

[codecov]

Codecov Report

Merging #1402 (254067c) into main (1e0041a) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1402      +/-   ##
==========================================
+ Coverage   62.22%   62.34%   +0.12%     
==========================================
  Files         316      317       +1     
  Lines       24317    24419     +102     
==========================================
+ Hits        15131    15224      +93     
- Misses       9186     9195       +9     

Impacted Files	Coverage Δ
src/systems/thruster/Thruster.cc	92.36% <100.00%> (ø)
include/ignition/gazebo/detail/View.hh	97.89% <0.00%> (-2.11%)	⬇️
src/SystemManager.cc	98.36% <0.00%> (-1.64%)	⬇️
src/systems/physics/Physics.cc	65.76% <0.00%> (-0.08%)	⬇️
src/SimulationRunner.cc	91.98% <0.00%> (-0.06%)	⬇️
src/SystemInternal.hh	100.00% <0.00%> (ø)
src/SimulationRunner.hh	100.00% <0.00%> (ø)
include/ignition/gazebo/detail/BaseView.hh	100.00% <0.00%> (ø)
include/ignition/gazebo/EntityComponentManager.hh	100.00% <0.00%> (ø)
src/EntityComponentManagerDiff.cc	81.25% <0.00%> (ø)
... and 1 more

---

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 1e0041a...254067c. Read the comment docs.

[clydemcqueen]

Thanks @arjo129. I will add the additional documentation and tests.

[clydemcqueen]

I think this addresses all of the feedback:

• I added a test to cover the <use_angvel_cmd> case with a negative <thrust_coefficient>.
• I updated the documentation in Thruster.hh to note that a negative <thrust_coefficient> means clockwise rotation.

I also made a few other changes to the documentation in Thruster.hh:

• There were conflicting comments as to the default for <use_angvel_cmd>, I verified that it is false and updated the comment.
• I noted that the parameters <min_thrust_cmd> and <max_thrust_cmd> also apply to angular velocity in the <use_angvel_cmd> case. This is a bit awkward since the units are either N or rad/s. Also, the parameters should probably be named <min_cmd> and <max_cmd>, but deprecating the old parameters and adding new ones seemed like overkill.

I'm happy for other feedback.

[arjo129]

Thanks for these changes. They look good to me.

[chapulina]

LGTM, I just have a nitpick about the documentation

[clydemcqueen]

@chapulina please take another look. ignition_gazebo-ci-pr_any-homebrew-amd64 failed 1 test but for unrelated reasons? Thanks

[Rezenders]

Hello,

I am trying to build Ignition from source with this patch but I am having problems matching all the packages versions.
If I use collection-garden.yaml it gets everything from the main branches, it builds but then I have problems with packages integrating ros and ignition. I guess that is probably expected since the main branches receive updates daily.
So, what I have been trying to do is to figure out which specific commits I should checkout to build garden with this patch. Any suggestions?

[clydemcqueen]

Hello, @Rezenders

Please see PR#2 in bluerov2_ignition. It's not complete, but it does include the hashes you are looking for.

[Rezenders]

Thanks for the reply. I will try it.

[jeroenzwan]

Hi,

I am trying to also use this version with the hashes you referred to in a previous post. However, when I am compiling it seems to me that there are quite some files missing in the ign-common package. Above is a screenshot of the folders and the content of ImageHeightmap.hh. I am trying to compile using a system with Ubuntu 22.04 and ROS2 Humble.
Below you can see the error that happens when I try to build with colcon build --merge-install --cmake-args ' -DBUILD_TESTING=OFF' ' -DCMAKE_BUILD_TYPE=Release'

Do you have any suggestions?

[mjcarroll]

You are missing the common::graphics component due to a lack of freeimage, sudo apt install libfreeimage-dev should fix it.

[jeroenzwan]

Thanks @mjcarroll for your reply. However, I still have the same error.

[jeroenzwan]

Update: I was indeed missing libfreeimage-dev, so I went through installing all the dependencies again and found I was missing some others as well. This resolved the issue, thanks for the help!