-
Notifications
You must be signed in to change notification settings - Fork 253
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
Make WindEffects configurable on a location basis #1357
Conversation
There are a few questions that remain to be answered:
On a related note, I couldn't easily run |
FYI @caguero |
Talking to @azeey earlier today, he rightly pointed out that simply applying forces to link frame origins does not allow for torques to be developed at the interface between regions. Then the question is how we model wind forces. Right now we make them proportional to the link mass and linear velocity w.r.t that of the wind. We could compute the solid geometries that result from the intersection between a link's collision geometry and each region (a là CSG), assume homogeneous density, and apply the same model to each solid geometry independently. I wonder though, if forces proportional to some cross-section area normal to the direction of the wind wouldn't give us "better" results (the definition of good, let alone better, is quite blurry to me). |
Hmm, a piecewise vector field for wind velocity would indeed be simpler to reason about than this scaling factor (which is bound to the approximation model). |
I had wondered about this actually. It would seem to me that there is some formulation that will be similar to the hydrodynamics formulation where we can capture the changing faces on the behaviour of the wind. However, I think we don't need to go to this detail at all for the LRAUV. One thing to note is that the LRAUV will experience torque given its CG is not the same as the link so for now I think we can get away with what we have implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I definitely recommend to move
Polynomial3
,Region3d
,SeparableFunction3d
, andPiecewiseScalarField3d
to Ignition Math. Overall, it'll make the review easier to follow, as we'll be splitting everything into multiple PRs. -
I noticed a few style inconsistencies. E.g.: Not using
this->
for every member or function, not using the function separators (////////////////////////////////////////////////
) and not using Doxygen in the function declarations. -
I agree that maybe we can live with the fact that some torques won't be generated if we apply the force at the link origin.
-
We probably need to parse the SDF data in this plugin, and then, pass the numeric values to the Ignition Math classes that we use.
-
I recommend to document the new SDF parameters in the
WindEffects.hh
file. Open it to see how the current parameters are described. It'll be infinite more useful if you include an example and explain what you're trying to achieve and how that translates into the SDF parameters. It could be your test but explained for a human.
4f41954
to
ad4b5ab
Compare
@caguero See gazebosim/gz-math#387. That PR can of course be further split.
@caguero I think I've addressed this, though I haven't added Doxygen documentation to private functions that are implementation details (which are fairly self explanatory on their own already).
@caguero Fair enough.
@caguero Done.
@caguero Done.
@caguero We have class documentation and regression tests. We can add an example too if you think it's worth it. |
Its <force_approximation_scaling_factor> is now a piecewise scalar field configurable from the SDF file. Backwards compatibility has been observed. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
68310f3
to
a6ac58b
Compare
Rebased to resolve conflicts. |
https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-homebrew-amd64 appears to be broken in |
Codecov Report
@@ Coverage Diff @@
## main #1357 +/- ##
==========================================
+ Coverage 62.34% 62.39% +0.05%
==========================================
Files 317 317
Lines 24419 24489 +70
==========================================
+ Hits 15224 15281 +57
- Misses 9195 9208 +13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now.
Thanks @arjo129 ! @chapulina how do we go about failing CI jobs? Do we merge despite them or do we quarantine PRs until that's solved? |
Everything that's |
🎉 New feature
Closes #1354.
Summary
This patch turns the force approximation scaling factor in the
WindEffects
plugin into a piecewise scalar field, configurable from the SDF file. It can still be configured as a scalar, so it remains backwards compatible.Test it
An integration test case has been added to the
INTEGRATION_wind_effects
test suite.Checklist
codecheck
passed (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-by
messages.