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.

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
@sebcrozet sebcrozet deleted the more_conversions branch April 27, 2021 11:40
@Andlon
Copy link
Sponsor Collaborator

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 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 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
Sponsor Collaborator

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

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
Sponsor Collaborator

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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants