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

Migration of maliput viewer plugin #377

Closed
agalbachicar opened this issue Apr 6, 2021 · 13 comments
Closed

Migration of maliput viewer plugin #377

agalbachicar opened this issue Apr 6, 2021 · 13 comments

Comments

@agalbachicar
Copy link
Collaborator

agalbachicar commented Apr 6, 2021

Parent issue: #332

EDITED: Go to Labor breakdown <=

This plugin is big enough to deserve its own issue. Below we provide a description of the status of the plugin:

  • maliput_mesh: standalone library that converts from maliput::utility::mesh::GeoMesh to ign-common3 ignition::common::Mesh.
  • maliput_viewer_widget: main controller of the maliput plugin. It manages a handful of widgets
    • maliput_viewer_model: holds the RoadNetwork and provides support for queries that the widgets perform.
    • layer_selection_widget: has a widget to select which mesh to display, a widget to toggle lane and branchpoint label visualization, and a widget to load files for a road network loader.
    • render_maliput_widget: loads the road meshes (different layers provided by maliput::utility::mesh`), manages their visuals, and the logic when selecting multiple meshes.
    • rule_visualizer_widget: visualizes the rules in a few text boxes.
    • arrow_mesh: draws cones where the click button maps to an inertial frame position into the scene.
    • selector: manages the lane selection visualization
    • orbit_view_control: manages the camera events based on mouse inputs in the scene.
    • traffic_light_manager: creates and manages traffic lights

We need to migrate this plugin to use the newer ign libraries.


Current maliput_viewer:

image

@agalbachicar
Copy link
Collaborator Author

Architecture alternatives

Option 1: multiple plugins talking to each other

Description: one core plugin to load the RoadNetwork and to answer queries to it. Multiple plugins to visualize information (now widgets).

PROs:

  • Separation of concerns.
  • The information management is only accesible via ign-transport which makes migration to newer versions straightforward
  • Each widget will be independent from each other.
  • Easier migration (duplicate functionality can coexist because code re-usage will be minimum).
  • Easier to test.

CONSs:

  • Mesh management. Making a message out of a mesh does not make sense, dumping a mesh to a file and passing the path might not make sense either.
  • Considerable amount of new work:
    • New sync mechanism via ign-transport.
    • Code duplication.

Option 2: same structure as of today but with new widget representations

Description: will keep the backend of the widget but change the UI to use QML and the new tooling offered by ign-gui and ign-rendering.

PROs:

  • No extra burden with plugin communication.
  • Less migration work (it should imply a refactor in the existing code to decouple Qt / Ign code from backends and just a new UI implementation).

CONSs:

  • Harder to maintain because of monolithic implementation.
  • Harder to test.

Migration caveats

  • The Scene3D plugin provides the orbit_view_control (no need to implement) but does not allow us to hook to mouse events in the scene. We should implement this separately and in a hardcoded fashion (scene by name) in the maliput viewer plugin configuration.

Preference

Given that the the second option might imply less work than the other, we incline towards option 2.

@agalbachicar
Copy link
Collaborator Author

agalbachicar commented Apr 6, 2021

Option 2 - Labor breakdown

Total time estimation: 20 days of work per FTE

@scpeters
Copy link
Contributor

scpeters commented Apr 6, 2021

Option 1: multiple plugins talking to each other

PROs:

  • Easier migration (duplicate functionality can coexist because code re-usage will be minimum).

Option 2: same structure as of today but with new widget representations

PROs:

  • Less migration work (it should imply a refactor in the existing code to decouple Qt / Ign code from backends and just a new UI implementation).

perhaps we can rephrase one of these bullet points because they currently seem to both say that they are the easier migration path. For Option 1, should we say "Incremental migration" instead of "Easier migration"?

@agalbachicar
Copy link
Collaborator Author

I see your point. Let me try to expand a bit what we discussed with @francocipollone .

Option 1 will imply multiple plugins, one independent from the other what makes it simpler. Each new feature will not be added a monolithic plugin that shares code a significant amount of code with the old ign deps.

Option 2 implies less duplicated code which in turns ends up being less code to write but might require some refactor in what currently exists.

I think we can achieve an incremental migration with both options. Thoughts?

@scpeters
Copy link
Contributor

scpeters commented Apr 6, 2021

we should also keep in mind that the Scene3D plugin was duplicated into ign-gazebo and has had some changes there:

I had to port some changes from ign-gazebo to fix the material loading for Scene3D in ign-gui (gazebosim/gz-gui#191). I can look to see if the RayCast support is different in ign-gazebo and is worth porting to ign-gui

@scpeters
Copy link
Contributor

scpeters commented Apr 6, 2021

for Option 1: perhaps we can say it is easier to maintain?

@francocipollone
Copy link
Collaborator

[Task 5]: Provide Ray-cast support.

Isuee found

In order to provide raycast suport we need first to be able to catch the mouse event on the scene (Scene3D plugin) from the MaliputViewerPlugin . Scene3D is managing the mouse events through internal handlers and there is no way to hook to this event.

Moving to a solution

I've modified ignition::gui::plugins::Scene3D plugin (from a fork) to emit a signal when a mouse event occurs, that way from MaliputViewerPlugin we can hook to that event.
--> commit: Emit signal when mouse press event occurs

Demo:

mouse_event.mp4

Pushing this change to upstream

I've suggested this change to the ignition folks, they'd already opened a ticket to enhance this behavior.
--> gazebosim/gz-gui#209 (comment)

@francocipollone
Copy link
Collaborator

Updates on [Task 5]: Provide Ray-cast support.

Inspired a bit on the open PR in ignition gui and digging deeper on the QT event system I realized there is a way to hook to the
MouseEvents that take place within the Scene3D plugin that is managing the main scene. So there is no need to wait until the PR in ign-gui is merged. We can move forward.

This is implemented in #388

@francocipollone
Copy link
Collaborator

francocipollone commented May 3, 2021

[Task 7]: Select lanes.

This task was split into two tasks/PRs.

Tasks:

@francocipollone
Copy link
Collaborator

francocipollone commented May 5, 2021

We missed one task which is visualize the phase for the selected lanes --> I've added it to the labor breakdown

ps: I am taking task 8

@francocipollone
Copy link
Collaborator

francocipollone commented May 7, 2021

[Task 9]: Select phase #398

This task would imply:

  • RoadRulebook and PhaseRing book should be loaded from YAML files.

  • UI for selecting a phase for a particular phase ring (tree structure).
    For instance, for the TShapeRoad:
    image

  • Dynamic rules should change when phase change.

Appendix

A simple road rulebook + phase ring book for the TShapeRoad xodr could be the following.
Note: inspired on TShapeRoad.yaml but no traffic lights were added.

# -*- yaml -*-
---
RoadRulebook:
  RightOfWayRules:
    - ID: WestApproach
      States:
        Go: []
        Stop: []
      Zone:
      - Lane: "1_0_1"
      ZoneType: StopAllowed
    - ID: EastApproach
      States:
        Go: []
        Stop: []
      Zone:
      - Lane: "0_0_-1"
      ZoneType: StopAllowed
  DirectionUsageRules: []

PhaseRings:
- ID: TIntersectionPhaseRing
  Rules: [EastApproach, WestApproach]
  Phases:
  - ID: AllGo
    RightOfWayRuleStates: {EastApproach: Go, WestApproach: Go}
  - ID: AllStop
    RightOfWayRuleStates: {EastApproach: Stop, WestApproach: Stop}
  PhaseTransitionGraph:
    AllGo:
    - ID: AllStop
      duration_until: 45
    AllStop:
    - ID: AllGo

Intersections:
- ID: TIntersection
  PhaseRing: TIntersectionPhaseRing
  InitialPhase: AllGo

@francocipollone
Copy link
Collaborator

MaliputViewer plugin is completely migrated 🥂

@scpeters
Copy link
Contributor

amazing work!

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

No branches or pull requests

3 participants