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

[GDScript 2.0] Remove confusing functions from Transform2D #62706

Closed
AThousandShips opened this issue Jul 4, 2022 · 2 comments · Fixed by #70995
Closed

[GDScript 2.0] Remove confusing functions from Transform2D #62706

AThousandShips opened this issue Jul 4, 2022 · 2 comments · Fixed by #70995

Comments

@AThousandShips
Copy link
Member

Godot version

4.0.dev

System information

Windows 10

Issue description

Built-in types are returned by value in GDScript, meaning that any modification of these (unless they are ref-counted like Array and Dictionary) by functions is ignored except when accessed as a variable itself, this seems to be reflected in how (mostly) GDScript does not expose such functions even when they exist in the class (like rotate on Transform2D/3D), however there are three exceptions to this: set_rotation set_scale set_skew on Transform2D

These can cause confusion (I for one was confused) as it is not obvious that functions called on a Transform2D cannot modify it when accessed in an array or dictionary or when from a Node2D etc., and to me it looks that these cases were overlooked

I would suggest simply removing these functions from GDScript, alternatively replacing them with non-modifying functions with_rotation with_scale with_skew if the functionality is considered important enough to keep

Steps to reproduce

To illustrate this:

var tr = Transform2D()
var arr = [Transform2D()]
print(tr)
print(arr)
tr.set_scale(Vector2(2, 2))
arr[0].set_scale(Vector2(2, 2))
print(tr)
print(arr)

And note how the transform in the array is unaffected

Minimal reproduction project

No response

@Calinou
Copy link
Member

Calinou commented Jul 4, 2022

Related to godotengine/godot-proposals#1336.

@Calinou Calinou added this to the 4.0 milestone Jul 4, 2022
@Calinou Calinou moved this to To Assess in 4.x Priority Issues Jul 4, 2022
@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 4, 2022

The set_rotation function was added in #47872 and I'm not sure why as it's never called by name there, and the rest were exposed by #52398 reasoning that the rotation one was already exposed

And to me it looks like set_rotation was only exposed there because it was used in the test project, I don't know about other areas where exposing it is justified

@reduz reduz moved this from To Assess to In Progress in 4.x Priority Issues Jan 6, 2023
reduz added a commit to reduz/godot that referenced this issue Jan 6, 2023
Fixes godotengine#62706.
Code is commented instead of removed to clarify why they should not be re-added.
@github-project-automation github-project-automation bot moved this from In Progress to Done in 4.x Priority Issues Jan 6, 2023
Streq pushed a commit to Streq/godot that referenced this issue Feb 9, 2023
Fixes godotengine#62706.
Code is commented instead of removed to clarify why they should not be re-added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants