search

tesseract-robotics / tesseract_gui

Tesseract Qt UI Components
GNU Lesser General Public License v3.0
4 stars 2 forks source link

Move `tesseract_widgets` into `tesseract` repository #23

Closed marip8 closed 2 years ago

marip8 commented 2 years ago

Per the title, it would be nice to move the widgets into the main tesseract repository. This way users do not have to build the tesseract_robotics_studio and ignition related components of this repository just to use the widgets. This would also make the tesseract_ros and tesseract_ros2 builds simpler

Levi-Armstrong commented 2 years ago

I am not opposed to this but it would need to go in tesseract_planning because it depends on planning classes. Also we could make it a separate repository. @marip8 @mpowelson What are your thoughts?

marip8 commented 2 years ago

I think it would make sense to move the "core" widgets into tesseract and the planning widgets into a similar package in tesseract_planning (e.g., tesseract_planning_widgets).

Alternatively, you could put each widget in the same package as the object it represents. That might be a little more intuitive

Levi-Armstrong commented 2 years ago

I think the two options are to either put tesseract_widgets in the tesseract_planning repository or make it its own repository. The original reason behind creating tesseract_planning was it was more unstable and that eventually it would merge back into tesseract repo once it became more stable.

marip8 commented 2 years ago

I would prefer to keep all of the GUI components separate to not add Qt dependencies on the individual packages.

Fair enough. It would be fairly simple to add a CMake argument (i.e., TESSERACT_BUILD_GUI) around the compilation of the Qt dependent libraries in each package/module too if that makes a difference.

From a maintenance stand point I would prefer to keep all of the GUI components in a single package.

I think this makes sense when the widgets live in the same repository as the objects they represent. If someone makes a change to an object that breaks its widget, I think it's easier to maintain if the widget isn't in some other repository that the person has to hunt down and make a corresponding PR against. That's why I would like to see the core widgets go into tesseract and the planning widgets go into tesseract_planning. It would also be nice for people who are not using tesseract_planning to also have access to the core widgets without having to build all the planners.

I guess the trade-off would be handling changes to the core widgets that break the planning widgets if they become separated. I'm not sure how tightly coupled they are, but that seems like an acceptable trade-off. We're already doing that with the ROS repos and it works okay

Levi-Armstrong commented 2 years ago

Fair enough. It would be fairly simple to add a CMake argument (i.e., TESSERACT_BUILD_GUI) around the compilation of the Qt dependent libraries in each package/module too if that makes a difference.

Yea from the cmake point it is possible but I believe this is not possible in package.xml.

Levi-Armstrong commented 2 years ago

I think this makes sense when the widgets live in the same repository as the objects they represent. If someone makes a change to an object that breaks its widget, I think it's easier to maintain if the widget isn't in some other repository that the person has to hunt down and make a corresponding PR against. That's why I would like to see the core widgets go into tesseract and the planning widgets go into tesseract_planning. It would also be nice for people who are not using tesseract_planning to also have access to the core widgets without having to build all the planners.

I guess the trade-off would be handling changes to the core widgets that break the planning widgets if they become separated. I'm not sure how tightly coupled they are, but that seems like an acceptable trade-off. We're already doing that with the ROS repos and it works okay

Yea in some case the widgets align with individual packages and in others they do not. Also there are other GUI libraries other than Qt which is another reason I prefer to keep Qt out of the core packages. I will create a tesseract_qt repository and move the tesseract_widget there.

Levi-Armstrong commented 2 years ago

The package tesseract_widget and been moved to tesseract_qt and everything renamed. Also tesseract_ros has been updated to only require this removing the dependency on ignition.