ros-industrial / universal_robot

ROS-Industrial Universal Robots support (https://wiki.ros.org/universal_robot)
1.05k stars 1.03k forks source link

ur_description and ur_gazebo are improved and simplified #624

Open BobbyCephy opened 1 year ago

BobbyCephy commented 1 year ago
gavanderhoorn commented 1 year ago

Thanks for the PR.

Let me start by a comment on form, not function.

The PR consists of a single commit, changing the following this (from your commit comment):

It touches 98 files.

In its current form, this is difficult to review.

I would suggest to split it up into multiple commits, each introducing (a) coherent set(s) of changes.

As an additional comment, let me also say that (some of) the duplication here was on purpose: in my years of maintaining (ROS) software, I've noticed (and have had the feedback) that the more parameters or arguments something takes, the complexer things become.

Users with programming experience typically do not mind too much, as they are used to variability and configuring that variability using parameters (arguments to functions fi).

Users with other backgrounds tend to appreciate a single file which caters to their use-case, with other files for other use-cases.

It's very clear you have to work with files ending in _ur3 fi if you have a UR3.

At first glance, this PR seems to go in the more-parameters direction. That's not necessarily a bad thing, but I do believe we should be mindful of the complexity of things and how changes like these potentially impact the experience of users.

gavanderhoorn commented 1 year ago

I would perhaps also suggest to describe your motivation for this PR.

Why did you change things, and why did you change them in this particular way?

BobbyCephy commented 1 year ago

Thank you for your quick and extensive reply! I am going to split the commit in the suggested changes. The large number of touched files is mainly caused by replacing model specific with single generic ones. The only new parameter and argument is actually only model? I guess it is easier having only a few files in a single folder to check, which is needed and not many ones that are nearly the same. The only differing model argument should be quickly realized. The main advantage of few generic files is only having to apply changes at a single place, which makes maintaining a lot easier.

gavanderhoorn commented 1 year ago

The main advantage of few generic files is only having to apply changes at a single place, which makes maintaining a lot easier.

while I don't disagree, this is probably exactly where the trade-off lies.

Do we optimise for maintenance effort, or for user experience?

Given the fact these are ROS 1 xacro:macros, I'm not sure how much we gain by re-arranging things so late 'in the game'.

BobbyCephy commented 1 year ago

I think it is worth reducing the number of specific and generic files to a seventh by introducing one additional parameter. The user experience should be improved as well, since it is easier to get an overview of a small set of files. In my opinion it is never to late for improvements. Even if few benefit from them directly, one can learn from them in future projects. A compromise might be a script, that generates specific from generic files on demand, that are ignored by git?