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

Plotting Header time #115

Merged
merged 15 commits into from
Sep 10, 2020

Conversation

AmrElsersy
Copy link
Contributor

@AmrElsersy AmrElsersy commented Aug 24, 2020

Improving Plotting performance

  • Points Limit (setting max num of points per graph)
  • enabling OpenGL for rendering the points (significant performance improvement)

Plotting tool time update:

Plotting according to the time that is associated with the header field of the msg if exists

for example, plotting a topic with a header time = sim time, makes the plotting stops if the simulation stops

sim_time

@chapulina @claireyywang

@chapulina chapulina added enhancement New feature or request 🔮 dome Ignition Dome labels Aug 25, 2020
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
chapulina and others added 5 commits August 27, 2020 19:55
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working for me both for topics that have a header and for those which don't. Good job, Amr! I have some small comments below.


I saw this warning, not sure what I did to trigger it:

[GUI] [Wrn] [Application.cc:649] [QT] qrc:/qml/PlottingInterface.qml:328:19: Unable to assign double to QSize

include/ignition/gui/PlottingInterface.hh Show resolved Hide resolved
include/ignition/gui/qml/Chart.qml Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great upon testing. Just some nitpicks. Overall LGTM!

include/ignition/gui/qml/Chart.qml Show resolved Hide resolved
src/PlottingInterface.cc Show resolved Hide resolved
src/PlottingInterface.cc Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface.cc Outdated Show resolved Hide resolved
src/PlottingInterface_TEST.cc Outdated Show resolved Hide resolved
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin is not loading for me anymore, with this error:

[GUI] [Wrn] [Application.cc:649] [QT] qrc:/qml/PlottingInterface.qml:78: TypeError: Type error

@chapulina
Copy link
Contributor

This fixes the QML error for me, it's another one of those differences due to the javascript version:

diff --git a/include/ignition/gui/qml/Chart.qml b/include/ignition/gui/qml/Chart.qml
index 5403df2..7f3b7cd 100644
--- a/include/ignition/gui/qml/Chart.qml
+++ b/include/ignition/gui/qml/Chart.qml
@@ -485,7 +485,7 @@ Rectangle {
       add new series
       ID key of the series: path of the field of the series
     */
-    function addSeries(ID, seriesDisplayText = "") {
+    function addSeries(ID, seriesDisplayText) {
       var seriesName = (seriesDisplayText) ? seriesDisplayText : ID
       var newSeries = createSeries(ChartView.SeriesTypeLine, seriesName, xAxis, yAxis);
       newSeries.useOpenGL = true;

AmrElsersy and others added 2 commits September 4, 2020 04:02
Signed-off-by: AmrElsersy <amrelsersay@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! And it works with gazebosim/gz-sim#270 as well. Great work! Let's wait for @claireyywang 's review.

include/ignition/gui/qml/Chart.qml Show resolved Hide resolved
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2020
Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! [UPDATE] I triggered the homebrew CI again which seems to be reflected on the CI below. No extra link needed :D

@chapulina chapulina merged commit 24c75f5 into gazebosim:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🔮 dome Ignition Dome enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants