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

Fix incorrect name of internal Basis global scale getter #90748

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

aaronfranke
Copy link
Member

I was looking at the Basis scale methods to double-check that my review of #90584 was correct and I noticed a problem. Basis currently has an internal-only (not exposed) method called get_scale_local that gets the global scale.

Fortunately the fix is very simple: Rename the method. The 3 places using the method are already correct, they are expecting the global scale as they pass it into scale which is global (we could rename this too, although it might be confusing because scaled with a "d" is exposed and is also global).

Proof: Here is what happens if I expose the method and call it from GDScript:

extends Node


func _ready() -> void:
	var rot_90_xy = Basis(Vector3.UP, Vector3.LEFT, Vector3.BACK)
	var b = rot_90_xy * Basis.from_scale(Vector3(1, 2, 3))
	print(b.get_scale()) # Prints (1, 2, 3) the local scale
	print(b.get_scale_local()) # Prints (2, 1, 3) the global scale, so let's rename to `get_scale_global`

By the way, if you are wondering whether from_scale is global or local, it's actually both. The difference is that other * from_scale will perform a local scale, while from_scale * other will perform a global scale.

@aaronfranke aaronfranke added this to the 4.3 milestone Apr 16, 2024
@aaronfranke aaronfranke requested review from a team as code owners April 16, 2024 11:35
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, though I think we could use a more clear naming perhaps, depending on assumptions of it, maybe something like get_scale_transposed

@akien-mga akien-mga merged commit 61d146c into godotengine:master Apr 22, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the fix-basis-scale-global branch April 22, 2024 13:01
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.

3 participants