search

ubi-agni / human_hand

modular, articulated human hand model
Other
31 stars 6 forks source link

F#13 map prop files #14

Closed guihomework closed 1 year ago

guihomework commented 1 year ago

Solves #13 Since display.launch was already hiding some arguments of upload.launch I also did not add the new arguments to display.launch only to upload.launch. Also passing the path to layout files was not added at all as these settings are completely linked to the meshes that will remain part of the package at the contrary of mappings and sizing that depend on outside devices/settings.

This was tested in my own usage of upload.launch passing my own files and directly with display.launch which then used default settings.

rhaschke commented 1 year ago

Your approach is rather convoluted. Why don't you simply define the default where you define the argument as well? See #15 for this approach. Closing here in favor of #15.

guihomework commented 1 year ago

Since you ask a question, I answer here : my goal in a PR is to make it always backward compatible. I believe your PR #15 is not. Indeed, if somebody used the model/human_hand.urdf.xacro without using the robots/human_hand.urdf.xacro they would have to update to add the mandatory parameters, which I want to avoid. So my "convoluted" PR had a reason which has it sources in not affecting code outside this package without announcing it first with a warning. A warning permits to have external maintainers schedule their updates instead of forcing an immediate need of maintenance.

I suppose a proper warning is not possible outside xacro internal changes, but maybe by adding an explicit comment in the macros that won't get discarded at expand would at least leave a trace of that change. The simplification could then occur at a next release

rhaschke commented 1 year ago

My goal in a PR is to make it always backward compatible. I believe your PR #15 is not.

You are right, model/human_hand.urdf.xacro didn't declare the arguments. But it's trivial to add that. I pushed another commit.

... not affecting code outside this package without announcing it first with a warning.

I didn't find a warning. Could you please point out the corresponding line in code?

The simplification could then occur at a next release

What simplification do you refer to?

guihomework commented 1 year ago

I explained about a hypothetical scenario of changing code with a warning, and later doing your simplified code change that breaks external code. I did not add the warning myself yet as I do not know the best way to do that, it was also open for discussion, and you are the xacro expert (maybe there is a xacro command to trigger/add a deprecated/warning from user code ?)

I get that you don't like empty parameters and the if/unless system, and prefer to create the new variable with defaults immediately (which is then backward compatible). Even if I had the idea to create the new parameters with default, I still did not realize one did not need to pass the arguments through the macro (this is where you are the xacro expert and now all those tricks). Adding a default within the parameters of the macro seemed too complex (visually also not nice) to me.

I think I must just accept that I code convoluted. Sorry.

rhaschke commented 1 year ago

I explained about a hypothetical scenario...

This was not clear to me. However, I don't yet see the need for the warning. Do you suggest removing the default argument in future (which indeed should trigger a deprecation warning now)?

Is there a xacro command to trigger/add a deprecated/warning from user code?

Yes it is: xacro.[message,warning,error]