Skip to content
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

Tracks periodic changes in scene broadcaster #2010

Merged
merged 8 commits into from
Jul 18, 2023

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Jun 9, 2023

This commit proposes a change to the scene broadcaster which enables tracking of all components with periodic changes. This way if a component has a periodic change the scene broadcaster does not miss it.

For more info see this discussion #2001 (comment) where @azeey proposes this solution.

This commit is WIP (Work In Progress) and I still need to handle entity/component removal and add tests.

This commit proposes a change to the scene broadcaster which enables
tracking of all components with periodic changes. This way if a
component has a periodic change the scene broadcaster does not miss it.

For more info see this discussion #2001 (comment) where @azeey proposes this solution.

This commit is WIP and I still need to handle entity/component removal
and add tests.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
src/EntityComponentManager.cc Outdated Show resolved Hide resolved
src/systems/scene_broadcaster/SceneBroadcaster.cc Outdated Show resolved Hide resolved
* Removes clone made of BaseComponent.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129
Copy link
Contributor Author

arjo129 commented Jun 12, 2023

⚠️ Do not merge yet. Scene broadcaster test is failing. Will investigate why.

@azeey azeey moved this from Inbox to In progress in Core development Jun 12, 2023
@jrutgeer
Copy link
Contributor

@arjo129

Do not merge yet. Scene broadcaster test is failing. Will investigate why.

I did not check the test, but other than that it seems to work: I can publish position command messages and the pose is updated accordingly.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

❗ No coverage uploaded for pull request base (arjo/fix/1997@19d70c3). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 486a2ea differs from pull request most recent head 5c16828. Consider uploading reports for the commit 5c16828 to get more accurate results

@@               Coverage Diff                @@
##             arjo/fix/1997    #2010   +/-   ##
================================================
  Coverage                 ?   65.36%           
================================================
  Files                    ?      327           
  Lines                    ?    26929           
  Branches                 ?        0           
================================================
  Hits                     ?    17601           
  Misses                   ?     9328           
  Partials                 ?        0           

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129 arjo129 marked this pull request as ready for review June 15, 2023 04:42
@arjo129 arjo129 requested a review from mjcarroll as a code owner June 15, 2023 04:42
@arjo129 arjo129 requested a review from azeey June 15, 2023 04:44
@arjo129
Copy link
Contributor Author

arjo129 commented Jun 15, 2023

Test failures are unrelated. This looks like its good for a round of review.

arjo129 and others added 3 commits June 22, 2023 14:47
Co-authored-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjo129@gmail.com>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
…:gazebosim/gz-sim into arjo/fix/scene_broadcaster_track_changes
Core development automation moved this from In progress to In review Jul 18, 2023
@mjcarroll mjcarroll merged commit 47e9474 into arjo/fix/1997 Jul 18, 2023
10 of 12 checks passed
@mjcarroll mjcarroll deleted the arjo/fix/scene_broadcaster_track_changes branch July 18, 2023 13:56
Core development automation moved this from In review to Done Jul 18, 2023
arjo129 added a commit that referenced this pull request Aug 3, 2023
* Fix Joint Position Controller Behaviour Described in #1997 

There are several issues at play. First the target velcity calculation
was wrong as described in #1997. Secondly, even if we corrected that
there was still incorrect behaviour. This is due to the fact that we use
the PID's CmdMax to determine what the maximum velocity for the joint
is. However, in the event a user does not set a `<cmd_max>` this
defaults to zero and the joint does not move. Finally this PR updates
the tests. Previously our tests were only testing the case where the
position command was well above the position's maximum velocity, hence
it would slide into position. This PR introduces a test where we snap
the position of the joint into place instead.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Address PR feedback with regards to style.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Just one more thing

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* style

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* More minor style fixes

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* More minor style fixes

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Tracks periodic changes in scene broadcaster (#2010)

* Tracks periodic changes in scene broadcaster

This commit proposes a change to the scene broadcaster which enables
tracking of all components with periodic changes. This way if a
component has a periodic change the scene broadcaster does not miss it.

For more info see this discussion #2001 (comment) where @azeey proposes this solution.

This commit is WIP and I still need to handle entity/component removal
and add tests.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Rework changes

* Removes clone made of BaseComponent.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* More reworks of added APIs to ECM.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Fix tests

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Add ECM related tests.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Address spelling issue

Co-authored-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjo129@gmail.com>

* Get rid of TODO

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

---------

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjo129@gmail.com>
Co-authored-by: Michael Carroll <mjcarroll@intrinsic.ai>

* Migrate to new header.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Update test paths

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Update include/gz/sim/EntityComponentManager.hh

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo129@gmail.com>

* Rename methods

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* style

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Flipped data structure around

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

* Fix GCC warning

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

---------

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjo129@gmail.com>
Co-authored-by: Michael Carroll <mjcarroll@intrinsic.ai>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

JointPositionController in ABS mode has first-order behavior rather than setting the position
4 participants