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

More conversions #874

Merged
merged 6 commits into from Apr 27, 2021
Merged

More conversions #874

merged 6 commits into from Apr 27, 2021

Conversation

@sebcrozet
Copy link
Member

@sebcrozet sebcrozet commented Apr 27, 2021

Added

  • Conversion from an array [T; D] to an isometry Isometry<T, _, D> (as a translation).
  • Conversion from a static vector SVector<T; D> to an isometry Isometry<T, _, D> (as a translation).
  • Conversion from a point Point<T; D> to an isometry Isometry<T, _, D> (as a translation).
  • Conversion of an array [T; D] from/to a translation Translation<T, D>.
  • Conversion of a point Point<T, D> to a translation Translation<T, D>.
  • Conversion of the tuple of glam types (Vec3, Quat) from/to an Isometry2 or Isometry3.
  • Conversion of a glam type Vec2/3/4 from/to a Translation2/3/4.
sebcrozet added 6 commits Apr 27, 2021
Add [T; D] -> Isometry<T, R, D>
Add SVector<T, D> -> Isometry<T, R, D>
Add Point<T, D> -> Isometry<T, R, D>
Add [T; D] <-> Translation<T, D>
Add Point<T, D> -> Translation<T, D>
Add Isometry3 <-> (Vec3, Quat)
Add Isometry2 <-> (Vec3, Quat)
Add Translation2/3/4 <-> Vec2/3/4
@sebcrozet sebcrozet merged commit fb21476 into dev Apr 27, 2021
8 checks passed
8 checks passed
@github-actions
check-fmt
Details
@github-actions
clippy
Details
@github-actions
build-nalgebra
Details
@github-actions
test-nalgebra
Details
@github-actions
test-nalgebra-glm
Details
@github-actions
test-nalgebra-sparse
Details
@github-actions
build-wasm
Details
@github-actions
build-no-std
Details
@sebcrozet sebcrozet deleted the more_conversions branch Apr 27, 2021
@Andlon
Copy link
Collaborator

@Andlon Andlon commented Apr 27, 2021

I feel that perhaps some of these conversions are a bit too "aggressive"? For example, From<[[T; R]; C]> for SMatrix might cause a lot of confusion about the fact that the nested array

[[1, 2],
 [3, 4]]

is in fact not the matrix

[ 1, 2 ]
[ 3, 4 ]

but rather its transpose. Similarly, is it completely obvious that a Point is directly transformed into an Isometry? An Isometry is documented in terms of a translation, so I suppose implementing From for a vector makes sense. But a point is not a translation, it's a position. Sure, it may be interpreted as such, but doesn't that defeat the purpose of having a semantic distinction between Point and Vector in the first place?

@sebcrozet
Copy link
Member Author

@sebcrozet sebcrozet commented Apr 27, 2021

For example, From<[[T; R]; C]> for SMatrix might cause a lot of confusion about the fact that the nested array.

We also have From<[[T; R]; C]> implementation for SMatrix. So if there is an ambiguity, the compiler should catch it and generate an error requiring explicit type annotation. (EDIT: sorry, I misread your comment, see my other response)

Similarly, is it completely obvious that a Point is directly transformed into an Isometry? An Isometry is documented in terms of a translation, so I suppose implementing From for a vector makes sense. But a point is not a translation, it's a position. Sure, it may be interpreted as such, but doesn't that defeat the purpose of having a semantic distinction between Point and Vector in the first place?

I guess this point conversion may sound odd. However, isometries are very often used to represent a position (especially throughout our own code-bases). So I think this makes the Points -> Isometry relevant.

@sebcrozet
Copy link
Member Author

@sebcrozet sebcrozet commented Apr 27, 2021

Sorry, I misread your comment about From<[[T; R]; C]>. Yeah, I understand this can cause a confusion. However it appears that this conversion already existed before (see the impl_from_into_asref_2D macro partially removed by this PR); we are just making its implementation nicer here.

@Andlon
Copy link
Collaborator

@Andlon Andlon commented Apr 27, 2021

Similarly, is it completely obvious that a Point is directly transformed into an Isometry? An Isometry is documented in terms of a translation, so I suppose implementing From for a vector makes sense. But a point is not a translation, it's a position. Sure, it may be interpreted as such, but doesn't that defeat the purpose of having a semantic distinction between Point and Vector in the first place?

I guess this point conversion may sound odd. However, isometries are very often used to represent a position (especially throughout our own code-bases). So I think this makes the Points -> Isometry relevant.

Fair enough, I don't use isometries that often.

Sorry, I misread your comment about From<[[T; R]; C]>. Yeah, I understand this can cause a confusion. However it appears that this conversion already existed before (see the impl_from_into_asref_2D macro partially removed by this PR); we are just making its implementation nicer here.

Ah. Hmm, well, in that case I would even suggest to remove it altogether, in favor of a method that makes the storage order explicit. I.e. from_col_major_array or similar.

@sebcrozet
Copy link
Member Author

@sebcrozet sebcrozet commented Apr 27, 2021

Ah. Hmm, well, in that case I would even suggest to remove it altogether, in favor of a method that makes the storage order explicit. I.e. from_col_major_array or similar.

Yeah, that would definitely make sense (and would allow us to mark this from_col_major_array as const).

@Andlon
Copy link
Collaborator

@Andlon Andlon commented Apr 27, 2021

As a final comment, upon further reflection I think a better name would even be from_col_major_nested_array, because from_col_major_array could also indicate that the data is laid out in a flat array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants