Adding tests for hydrodynamics #1617
Conversation
quarkytale
added
tests
quarkytale
force-pushed
the
quarkytale/hydrodynamics_tests
branch
from
73728b4 to
df60e57
Compare
quarkytale
force-pushed
the
quarkytale/hydrodynamics_tests
branch
from
df60e57 to
b3da1a7
Compare
Signed-off-by: Dharini Dutia <[email protected]>
quarkytale
force-pushed
the
quarkytale/hydrodynamics_tests
branch
from
b3da1a7 to
e96d7ad
Compare
Signed-off-by: Dharini Dutia <[email protected]>
Signed-off-by: Dharini Dutia <[email protected]>
Signed-off-by: Dharini Dutia <[email protected]>
Signed-off-by: Dharini Dutia <[email protected]>
Signed-off-by: Dharini Dutia <[email protected]>
Signed-off-by: Dharini Dutia <[email protected]>
Signed-off-by: Dharini Dutia <[email protected]>
There was a problem hiding this comment.
The test expectations seem reasonable. You might want to put in a disclaimer that added mass was flipped as thats a behaviour change. In any case we should look at deprecating added mass here in Garden and onwards. I'll follow up with a few more tests from my side next week.
| auto dt = (double)_info.dt.count()/1e9; | ||
| auto alpha = 0.9; | ||
| stateDot = alpha * (state - this->dataPtr->prevState)/dt | ||
| + (1-alpha) * this->dataPtr->prevStateDot; |
There was a problem hiding this comment.
I'm curious, what's the reason to filter stateDot here?
There was a problem hiding this comment.
This is to smoothen the rate of change of velocity, we can directly capture WorldLinearAccelerationt too.
There was a problem hiding this comment.
I'm saying because having the rolling average filter here means that stateDot depends on all previously visited states (rather than just the last one). I'm not sure if or how much we want to worry about this, though.
There was a problem hiding this comment.
That's a valid point. It was discussed in the Buoy Sim project that this might introduce lag and instability.
@arjo129 How should I proceed here? I'm thinking to get this in to be at par with the current changes (though I want to make sure this is generally sound and not specific for LRAUV). And have a separate effort to make this suitable for buoy as well.
There was a problem hiding this comment.
We can go ahead and remove this. It will not effect LRAUV much. Alternatively we can add an alpha. Finally I thought we were deprecating the added mass within this plugin and switching to the new added mass formulation.
There was a problem hiding this comment.
So I compared the previous implementation, the filter and WorldLinearAcceleration and the previous implementation is definitely the most stable. Going with that and we can iterate on it in future, thanks!
Signed-off-by: Dharini Dutia <[email protected]>
quarkytale
force-pushed
the
quarkytale/hydrodynamics_tests
branch
from
40a39cb to
0fef948
Compare
Codecov Report
@@ Coverage Diff @@
## ign-gazebo6 #1617 +/- ##
===============================================
+ Coverage 64.54% 64.69% +0.14%
===============================================
Files 320 321 +1
Lines 25904 26055 +151
===============================================
+ Hits 16720 16855 +135
- Misses 9184 9200 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Dharini Dutia <[email protected]>
Summary
Updated the plugin with downstream changes from LRAUV Hydrodynamics plugin with the added mass contribution sign and smoothening
stateDotcalculation.Added tests for verifying:
Setup
Using basic shapes: sphere and cylinders in the test world.
To get hydrodynamic plugin parameters for the models,
2/5 MR^2, cylinder =1/2 MR^22/3 ρπR^3, cylinder =ρπR^2 L-> zDotWC=6πηR-> zWC=1/4 ρA->zWWResources
Test it
Run the test
./build/ignition-gazebo6/bin/INTEGRATION_hydrodynamicsor check the world directly
ign gazebo src/ign-gazebo/test/worlds/hydrodynamics.sdfChecklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.