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

Move axis parameter to modification instead of the modifier #27

Open
GrmanRodriguez opened this issue Apr 28, 2022 · 0 comments
Open

Move axis parameter to modification instead of the modifier #27

GrmanRodriguez opened this issue Apr 28, 2022 · 0 comments

Comments

@GrmanRodriguez
Copy link
Contributor

Right now the modifier class has an axis parameter that determines the axis of position and dimension modifications.

This parameter should instead be part of the modification itself, to have a better separation of concerns, increase code cohesion, and make the library easier to use.

For example, in the FixedOffsetModifier class there is a function that shifts the origin of the links and joints in the three dimensions. Since this is a method of the modifier class itself we shouldn't create more modifiers inside it, so it works like this:

original_modifier_axis = modifier.axis # save original modifier axis
modification = Modification()

modifier.axis = Side.X # change it for each axis you want to change

modification.add_position(value_x, absolute)
modifier.modify(modification)

modifier.axis = Side.Y

modification.add_position(value_y, absolute)
modifier.modify(modification)

modifier.axis = Side.Z

modification.add_position(value_z, absolute)
modifier.modify(modification)

modifier.axis = original_modifier_axis # restore original modifier axis

This is how it would look with the axis being in the modification:

modification = Modification()

modification.axis = Side.X 
modification.add_position(value_x, absolute)
modifier.modify(modification)

modification.axis = Side.Y
modification.add_position(value_y, absolute)
modifier.modify(modification)

modification.axis = Side.Z
modification.add_position(value_z, absolute)
modifier.modify(modification)

The code is shorter and makes more intuitive sense.

This change would break existing code however, so I'm open to feedback on the idea @traversaro @CarlottaSartore @AlexAntn

Dod
The axis parameter belongs to the Modification class instead of Modifier

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

No branches or pull requests

1 participant