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

Event based infrastructure state update #68

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Jan 23, 2024

New feature implementation

Implemented feature

Part of open-rmf/rmf_simulation#114, to make the visualization more scalable for larger facilities.

Implementation description

This PR changes the rmf_visualization_building_systems to work with doors and lifts only publish an update when a new subscriber is detected or a state change takes place.
This required a few changes:

  • Increase the depth size of the door / lift subscriptions.
  • Always publish all the markers whenever a new state is received. This makes sure late publishers always receive the state of all infrastructure.
  • Always store the state of all doors. This has the benefit that now the plugin is slightly more "responsive" to map changes since the markers can be switched immediately when the map is changed, compared to previously that they would only be displayed when the next state update comes in.

Marked as a draft as I work on the PRs across the other repositories and make sure they all work well together.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova marked this pull request as ready for review February 29, 2024 08:56
@luca-della-vedova luca-della-vedova changed the title First draft of event based state update Event based infrastructure state update Feb 29, 2024
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member Author

This PR is now ready for review.
Similar to its counterpart in rmf_ros2, it should be a UX improvement independent of the event based logic. Specifically it will have:

  • Lower latency with door display when switching floors, since door data is now cached and we don't need to wait for the next state update to come before displaying a door.
  • Fixed a long standing issue where if the name of the floor is non existing we still display doors and lifts.

The main cost will be a larger payload on the rviz marker topic since we publish the whole state.

The CI failure seems due to vision_msgs not being in rolling and it seems to have been introduced around the time the 24.04 transition took place. I think it can be ignored for now and we can troubleshoot it if it sticks until the next rolling sync.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member Author

I'll temporarily mark this as draft while I figure out what to do with it. The changes are not necessary anymore now that the event based logic was reverted in rmf_simulation, so we can either leave rmf_visualization unchanged or just select the fixes and leave out the performance cost (serializing and sending the whole state in rviz)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant