-
Notifications
You must be signed in to change notification settings - Fork 254
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
Force Visualization #1898
base: gz-sim7
Are you sure you want to change the base?
Force Visualization #1898
Conversation
Hmmm the rebase didnt work as expected 💀. Going to be very hard for reviewers to review. I think cherry-picking would be an alternative. I don't use this fork anymore.
Can be a seperate PR. |
The rebase looks terrible because the target branch for the original PR is about 2y old. I rebased my fork of that on gz-sim7 first then made changes. If you were to rebase the original PR it looks a lot better. Or I could make this PR target main? It depends on how you’d like to keep the history. I’ll prob squash most of my changes into one or two commits. Could do the same for your original history which would clean things up? Update Switched base branch from |
f7eb126
to
3cc409c
Compare
7852902
to
0fa43d0
Compare
Codecov Report
@@ Coverage Diff @@
## gz-sim7 #1898 +/- ##
===========================================
+ Coverage 64.54% 64.58% +0.03%
===========================================
Files 347 348 +1
Lines 27777 27865 +88
===========================================
+ Hits 17930 17997 +67
- Misses 9847 9868 +21
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Updated to fix an issue where only one force per entity was visible. Link publishes a component that maintains a There are now methods to remove visuals and clear them when the GUI plugin unloads. Example usage with multiple forces applied to the MBARI buoy: |
@arjo129 - this might be working well enough for a first version? Could target either Some questions:
|
That's awesome I'll review it tomorrow. Will cherry pick the commits onto a new branch fi you don't mind.
Seems reasonable to follow sensors.
Yep. We can revisit this in some future PR.
Again we can slate this as "future work" |
Sure - no prob. I also have a branch based against
I'll look at the approach used - it will probably mean additions to the SDF for force providing system plugins. Adding a function to toggle publishing the wrench component in Link and some boiler plate to activate / deactivate that based on the duration since last publish in the system plugins should work. |
I think it'd be great if you opened the PR using the rebased branch.
…On Mon, Feb 20, 2023, 5:42 PM Rhys Mainwaring ***@***.***> wrote:
Will cherry pick the commits onto a new branch fi you don't mind
Sure - no prob. I also have a branch based against gz-sim7 as well here (
https://github.com/srmainwaring/gz-sim/tree/srmainwaring/7/forceviz) if
that helps. Both build on your original PR (but rebased).
Seems reasonable to follow sensors.
I'll look at the approach used - it will probably mean additions to the
SDF for force providing system plugins. Adding a function to toggle
publishing the wrench component in Link and some boiler plate to activate /
deactivate that based on the duration since last publish in the system
plugins should work.
—
Reply to this email directly, view it on GitHub
<#1898 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEEMQB6GBELG4OJ7D6GC2LWYM4BPANCNFSM6AAAAAAU5K6TDM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Actually this PR already uses the rebased branch (targeting |
Noticed a bug: if there are multiple instances of a plugin generating forces for the same same link, then only one will contribution will be picked up. Example: lift-drag plugin for the iris quadcopter where two instances of the LiftDrag system generate forces for the same link. Update Also - something very odd (again). The |
I'll update the branch on my side to be based in |
037448a
to
2734570
Compare
My understanding is that there are no breaking abi changes. IIRC Adding a member function should not lead to ABI incompatibilities. Same applies for components. We have added components in minor releases in the past (not ideal). Ideally we would target main given that we may want to go back and break the ABI, but given the way git and github work it would have been impossible to determine the changes for reviewers on the PR without cherry picking commits.
I can't think of a way to solve this without massive changes in other places. Perhaps we should provide an option within sdf of liftdrag, buoyancy and hydrodynamics to specify |
No, it would have been fine. I'd just rebase the entire history starting with your changes on git rebase --onto main gz-sim7 arjo/forceviz In any case either The issues with updates from the ECM are proving very confusing. The current version is broken, I fixed one thing (multiple forces for one entity), but seem to have broken the updates that were working when I was using a simple message as the component payload. It will take a bit more digging to figure out why the GUI is getting the updates but not finding them in the templated I'll push the debug code for the component serializer / deserializer (commented out) so you can see what I mean. I find it helps to set the RTF very low (RTF = 0.01 or less) when debugging to avoid getting swamped with messages. |
Yeah that might work - it's not super urgent as you can introduce another link (this is what the lift_drag example does which is why I didn't spot it initially). The biggest issue is the broken update - which I thought I had solved, but managed to break again. That's a blocker for this at the moment. Update 3aa106602 adds debugging info using the previous approach of a component containing a single |
- Use new message type msgs::EntityWrenchMap. - Disable components::EntityWrench and components::EntityWrenches. - Enable components::EntityWrenchMap. Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
- Remove unused components. - Remove debug / prototyping code from Link and VisualizeForces. Signed-off-by: Rhys Mainwaring <[email protected]>
Here's an approach along those lines that might work: srmainwaring@d192a43. I've also added throttling to the LiftDrag plugin on the publication side to reduce bandwidth usage. Figure: force visualization with namespaces applied to SkyCat TVBS where each prop has two force contributions and all other lift elements are associated with the base link. Probably for a follow-up PR, but I think it might be useful to move the force publication out of the Link:
As a tool this is quite useful already. I have been trying to get a stable version of a 6DoF omnicopter running in Gazebo: https://github.com/srmainwaring/SITL_Models/blob/ignition/omnicopter/Gazebo/docs/Omnicopter.md |
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.
Thanks for the amazing work @srmainwaring. I just tried it. Overall it looks great. There are still some minor annoyances that we have to deal with.
- UI doesn't load the first time. Not sure why but you have to collapse the panel and then re-open the panel. I'm not sure if the issue is with the code I wrote early on or some weird Qt/GzGui interaction. We can ticket this and fix it as we iterate through the feature.
- Gravity does not appear. I think this is trivial to solve, again we can revisit this in the future. I remember a throwaway branch (here: https://github.com/gazebosim/gz-sim/tree/arjo/feat/force_debug) where I simply iterate through inertial components. Again this can be an add-on feature.
Both of these can be added as follow up PRs.
@arjo129 this branch (https://github.com/srmainwaring/gz-sim/tree/arjo/forceviz-throttled) shows how we could move the force publishing out of the Link class in a follow up PR. It adds some features that are hard to deal with in the current version.
I've made the changes in the LiftDrag plugin, adding a ForcePublisher class which would be moved to it's own file if we wanted to proceed. |
Interesting. One alternative would be to have the forcedrag plugin handle the publication as opposed to a force publisher. That would remove the need for additional API. |
I think this is just blocked by a msgs release? If so, I can do a gz-msgs9 release |
@osrf-jenkins run tests please |
Signed-off-by: Ian Chen <[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.
gz-msgs9 was released and CI should be working now. I merged with gz-sim7
to fix ABI build.
Force visualization in lift_drag.sdf
works for me. I made some minor style comments and a suggestion about function naming.
/// \brief Last system update simulation time | ||
public: std::chrono::steady_clock::duration lastUpdateTime{0}; | ||
|
||
|
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.
remove extra line
|
||
public: void RemoveVisual(const std::string &_ns); | ||
|
||
public: void ClearVisuals(); |
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.
add doxygen for these functions
@@ -306,6 +306,11 @@ namespace gz | |||
const math::Vector3d &_force, | |||
const math::Vector3d &_torque) const; | |||
|
|||
/// \brief Sets the visualization label used by the force visualization. | |||
/// \param[in] _label The label used for force visualizations. | |||
public: void SetVisualizationLabel( |
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.
It's not clear what this function does from the name and doc. At first glance I thought this will visualize a text label next to the link in the gui. After reading through the code, I think it assigns a unique color for any force visualizations that have the same label string?
Maybe rename this to something more descriptive? e.g. SetForceVisualizationGroup
or SetForceVisualizationLabel
and add a brief description about what the arg does on the gui side.
*/ | ||
|
||
#ifndef GZ_SIM_GUI_VISUALIZECONTACTS_HH_ | ||
#define GZ_SIM_GUI_VISUALIZECONTACTS_HH_ |
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.
VISUALIZECONTACTS
-> VISUALIZEFORCES
@srmainwaring Have you had a chance to take a look at the feedback? |
On my todo list. Apologies for the delay, I'll try to incorporate the changes in the next few days. |
#2051 Has a nice visual. I wonder if we should extract the visualization component from it out else where. |
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.
can you fix the conflicts ? are you planning to finish this PR @srmainwaring ?
Hi @ahcorde - forgot about this PR - let me review what's needed to get it ready. It may not be in this branch, but I had a more general way of enabling / disabling the force visualisation that wasn't so intrusive when not required. The visuals from the apply force plugin look good, so I'll look at what's involved in adopting those while I'm at it. |
@srmainwaring do we need to close this PR and create a new one for the other approach you mentioned? |
🎉 New feature
Supersedes #1184.
Targets base branch
main
as there is a new method inLink.h
.Summary
This updates the previous PR #1184 and resolves some of the issues with that PR outlined in #1184 (comment).
Dependencies
Tasks
There are still a number of tasks to complete:
Tasks for this PR
Tasks for follow up PR(s)
Test it
The lift-drag example has been modified to help with testing. From the workspace folder run:
and select
Visualize Forces
from the GUI plugin menu.In another terminal set the rotor velocity:
The plugin may be configured in the GUI config with the element:
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.