-
Notifications
You must be signed in to change notification settings - Fork 488
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
Implemented Save Canvas Layout in Plot Plugin #2924
base: gazebo11
Are you sure you want to change the base?
Implemented Save Canvas Layout in Plot Plugin #2924
Conversation
Added Option to save / load canvas layout to the plot plugin. Based on new "AddVariable" due to some inconsistency when manually adding plots to the canvas. Uses same mechanism as "OnAddVariable", except there is no sender but the latest IncrementalPlot is being used from the list that can be retrieved by "Plots"
Thanks for your contribution and sorry for the delay in reviewing this. This will be a really useful feature. I've only had a superficial glance at this so far, and I plan to look deeper, but I did notice quite a few lines with whitespace changes that increase the line length beyond 80 characters, which does not follow our style guide. Do you mind reverting those whitespace changes? If you are unable to do so, I can push directly to this branch to assist. |
Hi Steve, thanks for your message. I will take care of the whitespace-issue. |
Adjusted code to fit the max 80 characters per line rule.
Hi Steve, |
Signed-off-by: Steve Peters <[email protected]>
I reverted your whitespace changes in 170bdcd, so the diff is much smaller now |
unsigned int _retVal = this->dataPtr->yVariableContainer->AddVariablePill( | ||
_variable, targetId); | ||
|
||
return _retVal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change can be reverted
@@ -487,6 +514,8 @@ unsigned int PlotCanvas::PlotByVariable(const unsigned int _variableId) const | |||
///////////////////////////////////////////////// | |||
void PlotCanvas::OnAddVariable(const std::string &_variable) | |||
{ | |||
// gzdbg << "PlotCanvas::OnAddVariable: " << _variable << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line can be removed
#include <mutex> | ||
#include <tinyxml2.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may need to link the GUI library against tinyxml2 with this chanege
printf("Error: %i\n", a_eResult); \ | ||
return a_eResult; \ | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this macro needed? I would revert it
|
||
_canvas_XMLElement = _canvas_XMLElement->NextSiblingElement("Canvas"); | ||
} // while (_canvas_XMLElement != nullptr) | ||
} // void PlotWindow::LoadPlotLayout() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code formatting in these two functions doesn't match the gazebo style
Added Option to save / load canvas layout in plot plugin.
The Plugin was missing the option to save / load the canvas layout and used variables, so you had to manually set them up after each restart.
The feature needed a new overloaded
AddVariable
method due to some inconsistency when manually adding plots to the canvas withAddPlot
. The method is based onOnAddVariable
, except there is no sender but instead using the latestIncrementalPlot
from the list that can be retrieved byPlots
todo: add automated testing
todo: check file when loading