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

Expose determinant in Transform2D, rename internal method #76311

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Apr 21, 2023

I usually use 3D. As a math nerd I was helping out a friend with some math in a 2D game and I was shocked that the determinant was not exposed to GDScript. This PR exposes it.

Discussion: Since this is not exposed yet, we could also rename it from basis_determinant to just determinant, since there is no other possible way to interpret the word "determinant" here. If this is desired I would be happy to add it to this PR, but I don't want to cause unnecessary friction so I haven't done so yet. basis_determinant is still clear and correct, it's just... kinda redundant and unnecessary.

3.x version: #76323

@MewPurPur
Copy link
Contributor

I agree both with exposing and renaming it. It doesn't have anything to do with the Basis class, does it?

@kleonc
Copy link
Member

kleonc commented Apr 21, 2023

Discussion: Since this is not exposed yet, we could also rename it from basis_determinant to just determinant, since there is no other possible way to interpret the word "determinant" here. If this is desired I would be happy to add it to this PR, but I don't want to cause unnecessary friction so I haven't done so yet. basis_determinant is still clear and correct, it's just... kinda redundant and unnecessary.

Well, determinant makes sense only for square matrices. And Transform2D is said to be a 2×3 matrix. Given that, Transform2D.determinant seems kinda incorrect. It could be interpreted as either determinant of the basis (the 2×2 part of the matrix, the first two columns), or as a determinant of the 3x3 matrix the given Transform2D represents (for which the third non-stored row is always assumed to be [0, 0, 1]). But for both of the interpretations the result is the same, so there's no discrepancy on that part.
I find Transform2D.basis_determinant would be more conceptually correct. But Transform2D.determinant is shorter and should be fine too, given the docs will clearly explain what it actually returns.
Conceptually e.g. Vector2.cross also makes no sense but we do have it. 🙃 So yeah, determinant + explanation in the docs should be fine.

I agree both with exposing and renaming it. It doesn't have anything to do with the Basis class, does it?

@MewPurPur It does not, it has to do with the basis part of the Transform2D.

@MewPurPur
Copy link
Contributor

MewPurPur commented Apr 21, 2023

Hmm, sure thing, but the determinant tells us what the area of a unit square would be under the matrix transformation and this is unaffected from changing the origin. For xform it's more relevant to tell this than for determinant.

@aaronfranke aaronfranke changed the title Expose basis_determinant in Transform2D Expose determinant in Transform2D, rename internal method Apr 21, 2023
@aaronfranke
Copy link
Member Author

aaronfranke commented Apr 21, 2023

Pushed an update to rename to determinant. One more argument in favor of the rename:

  • We also have orthonormalized, get_rotation, get_scale, get_skew, rotated_local, scaled_local, which only use the basis don't include "basis" in the name. The only other methods that do include it are the xform methods which are necessary to disambiguate it from a full transform using the origin.

@MewPurPur Great point, it makes perfect sense conceptually when thinking about how the area changes, which is not affected by a translation.

Conceptually e.g. Vector2.cross also makes no sense but we do have it. 🙃

It does make sense actually in fact you can even extend it into 4D.

@MewPurPur
Copy link
Contributor

MewPurPur commented Apr 21, 2023

I'd consider Vector2.cross a shorthand notation for a mathematical hack, same for all cross products that aren't 3D or 7D. ... But that's off-topic :P

@kleonc
Copy link
Member

kleonc commented Apr 22, 2023

Hmm, sure thing, but the determinant tells us what the area of a unit square would be under the matrix transformation and this is unaffected from changing the origin. For xform it's more relevant to tell this than for determinant.

@MewPurPur What I was saying is that without a square matrix there's no talk about the determinant in the first place. Determinant is defined only for square matrices. By saying "determinant" you're automatically referring to some square matrix (the 2x2 or 3x3 matrix I've mentioned in #76311 (comment)).


  • We also have orthonormalized, get_rotation, get_scale, get_skew, rotated_local, scaled_local, which only use the basis don't include "basis" in the name. The only other methods that do include it are the xform methods which are necessary to disambiguate it from a full transform using the origin.

@aaronfranke Yeah, I've realized this about get_scale too after writing my previous comment, didn't bother with editing my comment as I've already agreed that determinant is fine anyway.

Conceptually e.g. Vector2.cross also makes no sense but we do have it. upside_down_face

It does make sense actually

@aaronfranke Was its supposed to link to a video about determinant? I'd expect something Vector2.cross related based on the quote. 🙃 Anyway, if the link is correct then note that in the video he's referring only to linear transformations / square matrices.


I'd consider Vector2.cross a shorthand notation for a mathematical hack, same for all cross products that aren't 3D or 7D. ... But that's off-topic :P

The 2D case can be seen as a simplified 3D case with vectors' third component being zero, so it could be said to be a simplification and not really a hack though. Unless that's exactly what you call "a mathematical hack". 😄
(We're indeed going offtopic and I've already agreed Transform2D.determinant is fine too, so I'll cut it here 🙂)

doc/classes/Transform2D.xml Outdated Show resolved Hide resolved
doc/classes/Transform2D.xml Outdated Show resolved Hide resolved
@aaronfranke
Copy link
Member Author

@kleonc Oh, sorry, I linked the wrong video. I meant to link this video. But yeah we already agree :)

@aaronfranke aaronfranke added this to the 4.1 milestone Apr 23, 2023
@akien-mga akien-mga merged commit f64544a into godotengine:master Apr 24, 2023
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the t2d-basis-det branch April 24, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants