Plotting interface #89

AmrElsersy · 2020-06-29T17:53:21Z

The Plotting Interface is responsible for Plotting the transport msgs that will be used from ign-gui and ign-gazebo plugins

it accepts the dragged fields from the TopicViewer plugin to register the fields to a specific chart

This is a UML Diagram for the code

@chapulina
@claireyywang

chapulina

Left some initial comments 😉

include/ignition/gui/qml/Chart.qml

src/plugins/plotting/GuiPlotting.cc

src/plugins/plotting/CMakeLists.txt

include/ignition/gui/PlottingInterface.hh

include/ignition/gui/qml/Chart.qml

include/ignition/gui/PlottingInterface.hh

include/ignition/gui/resources.qrc

claireyywang

Some minor comments. I understand this is still a draft but it's easier to get things right in the beginning then grind it later. I'd recommend going through the rest of the code following the same style suggestions to avoid repetitive comments in the next review.

include/ignition/gui/PlottingInterface.hh

src/PlottingInterface_TEST.cc

include/ignition/gui/PlottingInterface.hh

Signed-off-by: Louise Poubel <[email protected]>

Suggestions to gazebosim#89

claireyywang

[Resolved] What commands do you run to test the gui plugin? I'm still getting qml error on my machine using the latest code.

[GUI] [Err] [Plugin.cc:136] Failed to instantiate QML file [:/TransportPlotting/TransportPlotting.qml].

claireyywang

Hey @AmrElsersy , I did a quick pass of review. It would be helpful if you could offer some quick instructions on how to test the plotting interface, like what simulation world did you use, what are some values that are draggable etc.

src/plugins/plotting/TransportPlotting.hh

src/plugins/plotting/TransportPlotting.cc

src/PlottingInterface_TEST.cc

src/PlottingInterface.cc

chapulina

Left some more comments 😉

src/plugins/plotting/TransportPlotting.hh

include/ignition/gui/qml/PlottingInterface.qml

include/ignition/gui/qml/Chart.qml

include/ignition/gui/qml/PlottingInterface.qml

src/PlottingInterface.cc

src/PlottingInterface_TEST.cc

src/PlottingInterface.cc

claireyywang

OSX CI test failures seem to be unrelated to this PR. Could they be flaky tests again @chapulina ? I thought we disabled the known flaky ones.

src/PlottingInterface_TEST.cc

claireyywang

nitpick

include/ignition/gui/PlottingInterface.hh

src/PlottingInterface.cc

include/ignition/gui/qml/PlottingInterface.qml

include/ignition/gui/qml/Chart.qml

include/ignition/gui/qml/PlottingInterface.qml

src/PlottingInterface.cc

include/ignition/gui/qml/PlottingInterface.qml

claireyywang

To circumvent windows CI failure, similar to #103.

Homebrew CI failure seems unrelated to this PR so we are good.

Otherwise everything looks good to me. What do you think @chapulina ?

src/PlottingInterface_TEST.cc

chapulina

Great work, Amr! I just have some final comments and then I think we're ready to merge!

Just for the record for anyone reading this in the future, there will be another PR which will add support for using sim time from message headers on the X axis.

include/ignition/gui/qml/PlottingInterface.qml

claireyywang

I added two comments to fix the window CI error: TransportPlotting.dll : fatal error LNK1120: 1 unresolved externals

Otherwise LGTM

src/plugins/plotting/TransportPlotting.hh

chapulina

LGTM! 🎉

⚠️ But before merging, we need to:

Fix DCO: you can do this by rebasing - be sure to use the latest master branch
Fix CI
Update the indentation on the QML files

Signed-off-by: AmrElsersy <[email protected]>

Signed-off-by: Louise Poubel <[email protected]>

claireyywang

LGTM

Signed-off-by: Louise Poubel <[email protected]>

claireyywang changed the base branch from master to plotting-ui-project June 29, 2020 19:08

chapulina added enhancement New feature or request 🔮 dome Ignition Dome labels Jun 29, 2020

chapulina requested review from chapulina and claireyywang July 13, 2020 21:40

chapulina assigned AmrElsersy Jul 13, 2020

chapulina reviewed Jul 13, 2020

View reviewed changes

claireyywang suggested changes Jul 13, 2020

View reviewed changes

AmrElsersy requested review from chapulina and claireyywang July 23, 2020 02:44

claireyywang marked this pull request as ready for review July 23, 2020 17:57

claireyywang changed the base branch from plotting-ui-project to master July 23, 2020 17:58

chapulina added a commit that referenced this pull request Aug 11, 2020

Suggestions to #89

8fa5fa6

Signed-off-by: Louise Poubel <[email protected]>

chapulina mentioned this pull request Aug 11, 2020

Suggestions to #89 AmrElsersy/ign-gui#1

Merged

AmrElsersy added a commit to AmrElsersy/ign-gui that referenced this pull request Aug 12, 2020

Merge pull request #1 from ignitionrobotics/chapulina/plotting-interface

86fa0d4

Suggestions to gazebosim#89

claireyywang reviewed Aug 12, 2020

View reviewed changes

claireyywang suggested changes Aug 14, 2020

View reviewed changes

chapulina reviewed Aug 14, 2020

View reviewed changes

AmrElsersy requested review from claireyywang and chapulina August 14, 2020 09:28

claireyywang suggested changes Aug 17, 2020

View reviewed changes

src/PlottingInterface_TEST.cc Outdated Show resolved Hide resolved

claireyywang reviewed Aug 17, 2020

View reviewed changes

include/ignition/gui/PlottingInterface.hh Show resolved Hide resolved

src/PlottingInterface.cc Show resolved Hide resolved

AmrElsersy requested a review from claireyywang August 18, 2020 11:00

claireyywang reviewed Aug 18, 2020

View reviewed changes

include/ignition/gui/qml/PlottingInterface.qml Outdated Show resolved Hide resolved

chapulina reviewed Aug 19, 2020

View reviewed changes

include/ignition/gui/qml/PlottingInterface.qml Outdated Show resolved Hide resolved

include/ignition/gui/qml/PlottingInterface.qml Outdated Show resolved Hide resolved

chapulina suggested changes Aug 19, 2020

View reviewed changes

include/ignition/gui/qml/Chart.qml Outdated Show resolved Hide resolved

include/ignition/gui/qml/PlottingInterface.qml Outdated Show resolved Hide resolved

src/PlottingInterface.cc Show resolved Hide resolved

include/ignition/gui/qml/PlottingInterface.qml Outdated Show resolved Hide resolved

AmrElsersy requested review from claireyywang and chapulina August 20, 2020 00:57

claireyywang reviewed Aug 20, 2020

View reviewed changes

include/ignition/gui/qml/PlottingInterface.qml Outdated Show resolved Hide resolved

AmrElsersy requested a review from claireyywang August 21, 2020 02:01

claireyywang reviewed Aug 21, 2020

View reviewed changes

src/PlottingInterface_TEST.cc Outdated Show resolved Hide resolved

src/PlottingInterface_TEST.cc Show resolved Hide resolved

AmrElsersy requested a review from claireyywang August 22, 2020 03:20

chapulina reviewed Aug 24, 2020

View reviewed changes

claireyywang suggested changes Aug 25, 2020

View reviewed changes

src/plugins/plotting/TransportPlotting.hh Show resolved Hide resolved

src/plugins/plotting/TransportPlotting.hh Show resolved Hide resolved

chapulina approved these changes Aug 26, 2020

View reviewed changes

Plotting Interface

a6c935b

Signed-off-by: AmrElsersy <[email protected]>

AmrElsersy force-pushed the plotting-interface branch from 2c5063a to a6c935b Compare August 26, 2020 04:16

AmrElsersy requested a review from claireyywang August 26, 2020 04:18

attempt at fixing visibility on Windows

bcfb672

Signed-off-by: Louise Poubel <[email protected]>

claireyywang approved these changes Aug 26, 2020

View reviewed changes

Use macro to disable Windows test, fix indentation

8f3b1b8

Signed-off-by: Louise Poubel <[email protected]>

claireyywang added this to the nal milestone Aug 27, 2020

chapulina merged commit c49da10 into gazebosim:master Aug 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plotting interface #89

Plotting interface #89

AmrElsersy commented Jun 29, 2020 •

edited

Loading

chapulina left a comment

claireyywang left a comment

claireyywang left a comment •

edited

Loading

claireyywang left a comment

chapulina left a comment

claireyywang left a comment

claireyywang left a comment

claireyywang left a comment •

edited

Loading

chapulina left a comment

claireyywang left a comment •

edited

Loading

chapulina left a comment •

edited by claireyywang

Loading

claireyywang left a comment

Plotting interface #89

Plotting interface #89

Conversation

AmrElsersy commented Jun 29, 2020 • edited Loading

chapulina left a comment

Choose a reason for hiding this comment

claireyywang left a comment

Choose a reason for hiding this comment

claireyywang left a comment • edited Loading

Choose a reason for hiding this comment

claireyywang left a comment

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

claireyywang left a comment

Choose a reason for hiding this comment

claireyywang left a comment

Choose a reason for hiding this comment

claireyywang left a comment • edited Loading

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

claireyywang left a comment • edited Loading

Choose a reason for hiding this comment

chapulina left a comment • edited by claireyywang Loading

Choose a reason for hiding this comment

claireyywang left a comment

Choose a reason for hiding this comment

AmrElsersy commented Jun 29, 2020 •

edited

Loading

claireyywang left a comment •

edited

Loading

claireyywang left a comment •

edited

Loading

claireyywang left a comment •

edited

Loading

chapulina left a comment •

edited by claireyywang

Loading