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

Add Translation.from_vector(vector) and Reflection.from_plane(plane) #544

Closed
yck011522 opened this issue May 10, 2020 · 6 comments
Closed

Comments

@yck011522
Copy link
Contributor

Feature Request

As a user, I think the function Translation.from_vector(vector) should be added. Both for the sake of completeness and convenience. Similarly Reflection.from_plane(plane).

Details

Maybe there is a better way to do this from a user's perspective:

T = Translation(my_vector_with_really_long_name.x, my_vector_with_really_long_name.y, my_vector_with_really_long_name.z)

For example, this function will be consistent with , Transformation.from_frame_to_frame(frame_from, frame_to

@romanarust
Copy link
Member

For the Translation, I don't see the point of adding multiple constructors, it always needs the same information to be constructed: 3 floats, independent if those are stored in a tuple, a list or a Vector.

T = Translation(Vector(1, 2, 3))
T = Translation((1, 2, 3))
T = Translation([1, 2, 3])
T = Translation(my_vector_with_really_long_name)

For the Reflection this is somewhat similar: it always needs a plane to be constructed (point, normal). Indeed here I see a point of discussion: Is the expected behavior of creating a Reflection rather

R = Reflection(Plane(point, normal))
R = Reflection((point, normal))

than (currently)

R = Reflection(point, normal)

@yck011522
Copy link
Contributor Author

@romanarust I believe my confusion come from the fact that as a user of the library, I do not want to dig into the inner implementation of the classes. And I believe the signature of methods and functions should do most of the story-telling about what they do.

Translation.from_vector(vector) is illustrative to what it consumes and what it produces without reading documentation or reading the code of the constructor.

How I understand your suggestion

If the T = Translation(my_vector_with_really_long_name) behavior is allowed and is expected. It should be documented in the constructor method or better, in the class description page. But I still prefer self-descriptive function names over reading docs.

How I generally feel the compas library is right now

Worse still is the current expectation for user to

  1. understand that the constructor can be duck-typed by passing translation (list of float) – a list of 3 numbers defining the translation in x, y, and z.
  2. understand that Vector class can be passes as if it is a list of 3 numbers. (This is not even documented in Vector Class Description Page.
  3. Conjure that T = Translation(my_vector_with_really_long_name) may be a solution
  4. Test to see if this actually does what you expect it to do (say, the three numbers are actually in the correct order)

For users who care only about code-that-works will not go into this trouble of testing and will simply write out the long ugly code just to be safe. In the light of helping user help themselves, I would advocate the following:

Self-descriptive function names > Reading docs > Conjure undocumented code use

@yck011522
Copy link
Contributor Author

Hum. @brgcode I thought you replied to this thread a few days ago? You suggested that this will be addressed via documentation?

If that is the decision, we can close this issue.

@tomvanmele
Copy link
Member

while reworking the docs i noticed some inconsistencies. reworking them still. will push out update asap...

@gonzalocasas
Copy link
Member

@yck011522 this one is done, right?

@yck011522
Copy link
Contributor Author

I believe the two proposed functions is already completed, see this:
https://compas-dev.github.io/main/api/generated/compas.geometry.Translation.from_vector.html
https://compas-dev.github.io/main/api/generated/compas.geometry.Reflection.from_plane.html

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

4 participants