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

Initial migration #1

Merged
merged 12 commits into from
Jan 7, 2020
Merged

Conversation

DavidMerzJr
Copy link

These packages have been in internal development at SwRI under the Apache License for several months. These commits represent the prep for release of a number of those packages in a cleaned-up, stripped-down manner. This is a clean "starting over" point, from which these packages can hopefully grow into a highly useful open-source tool.

@robinsonmm
Copy link

@jrgnicho have you had a chance to review this PR?

Copy link
Contributor

@jrgnicho jrgnicho left a comment

Choose a reason for hiding this comment

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

Please remove all instances of unrelated words in comments and header guards

@@ -0,0 +1,7 @@
# ROS-Install File

Copy link
Contributor

Choose a reason for hiding this comment

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

does opp stand for offline path planner?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. OPP had been selected before I stepped in to finish some refactoring. I would not be opposed to changing the initials to TOP (Toolpath Offline Planning), but I would rather do so in a second pull request.

plane.origin(1) = static_cast<double>(origin.y);
plane.origin(2) = static_cast<double>(origin.z);

return boost::make_optional(plane);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think calling boost::make_optional is necessary, the returned plane object will get casted into an optional I think

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure whether it is necessary, I inherited this code. Either way, I think that explicitly calling make_optional makes it more clear what is happening. I generally prefer fewer 'implied' or 'behind-the-screen' castings.

opp_area_selection/src/area_selector.cpp Outdated Show resolved Hide resolved
opp_area_selection/src/selection_artist.cpp Show resolved Hide resolved
opp_gui/include/opp_gui/utils.h Outdated Show resolved Hide resolved
opp_gui/include/opp_gui/tool_path_planner_panel.h Outdated Show resolved Hide resolved
opp_gui/include/opp_gui/tool_path_planner_panel.h Outdated Show resolved Hide resolved
@DavidMerzJr
Copy link
Author

@jrgnicho I have made changes to address your above requests.

#include "opp_gui/utils.h"
#include "opp_gui/widgets/tool_path_editor_widget.h"
#include "opp_gui/widgets/touch_point_editor_widget.h"
#include "ui_tool_path_planner.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this header in the same directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, I just realized this is the automatically generated header from the ui file

@jrgnicho jrgnicho merged commit 2fff173 into swri-robotics:master Jan 7, 2020
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

Successfully merging this pull request may close these issues.

4 participants