-
Notifications
You must be signed in to change notification settings - Fork 275
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
Adds support for ocean currents #800
Conversation
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
I added very minor tweaks in 20b57f9. |
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.
My main suggestion is to also support the ability to set the current via SDF. This way, we'll cover the most common use case without having to send any additional messages. Remember to document the extra parameter as well.
Signed-off-by: Arjo Chakravarty <[email protected]>
…-gazebo into arjo/ocean_currents Signed-off-by: Arjo Chakravarty <[email protected]>
I've added a
|
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.
Very minor request, other than that looks good.
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
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.
Nits.
Signed-off-by: Arjo Chakravarty <[email protected]>
CI looks passing. I think Windows is disabled (thanks to the awesome new CI documentation in ignitionrobotics/docs/ci.md!), and the failing tests on Mac I've seen before. The push test is complaining about PERFORMANCE_each, which... should be okay? I haven't tried the PR itself, but we should probably add some integration tests next time (if it's possible to test with some known numbers?), to make sure we don't break stuff with new features. (I will try it in Fortress once it's forward ported 😅 I don't have a local Edifice build at the moment.) |
DCO won't let me merge. @caguero I think you need to sign the last commit. |
Signed-off-by: Arjo Chakravarty [email protected]
🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸
🎉 New feature
Closes #
Summary
Adds support for ocean currents to the hydrodynamics plugin. It also adds an optional support for namespaces.
The behaviour of the
<namespace>
parameters is described as follows:Test it
To test run:
And apply a really strong current:
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge