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

Implemented Save Canvas Layout in Plot Plugin #2924

Open
wants to merge 4 commits into
base: gazebo11
Choose a base branch
from

Conversation

m-melchior
Copy link

@m-melchior m-melchior commented Jan 25, 2021

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 with AddPlot. The method is based on OnAddVariable, except there is no sender but instead using the latest IncrementalPlot from the list that can be retrieved by Plots
todo: add automated testing
todo: check file when loading

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"
@chapulina chapulina added 11 Gazebo 11 enhancement New feature or request labels Feb 2, 2021
@chapulina chapulina requested a review from scpeters February 22, 2021 19:31
@scpeters
Copy link
Member

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.

@m-melchior
Copy link
Author

Hi Steve, thanks for your message. I will take care of the whitespace-issue.
Cheers,
michael.

Adjusted code to fit the max 80 characters per line rule.
@m-melchior
Copy link
Author

Hi Steve,
I finally got to reformat the code, hope it fits now. Pls let me know if there's anything else I could do.
Cheers,
michael.

@scpeters
Copy link
Member

Hi Steve, I finally got to reformat the code, hope it fits now. Pls let me know if there's anything else I could do. Cheers, michael.

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;
Copy link
Member

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;
Copy link
Member

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>
Copy link
Member

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
Copy link
Member

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() {
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
11 Gazebo 11 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants