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

Rename some arguments called "position" #89721

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 20, 2024

When you connect Tree's empty_clicked argument to the same Tree or just any Control node, you will get a warning that the argument position of the newly generated callback shadows a property.
I went and renamed all arguments that could cause such warning.

Bugsquad edit: Closes godotengine/godot-proposals#5715

@KoBeWi KoBeWi added this to the 4.3 milestone Mar 20, 2024
@KoBeWi KoBeWi requested review from a team as code owners March 20, 2024 22:43
@Sauermann
Copy link
Contributor

resolve godotengine/godot-proposals#5715

has a broader scope than #78864

@AThousandShips AThousandShips requested a review from a team March 21, 2024 11:07
@AThousandShips
Copy link
Member

Wanting the dotnet team to take a look concerning renaming and compatibility, I forget the specifics on what constitutes breaking compatibility there (as this is a non-critical change)

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 21, 2024

AFAIK _input_event might break compatibility, because C# supports named arguments. Signal changes only affect newly created callbacks.

@raulsntos
Copy link
Member

Signal changes won't break compat in C#, lambda parameter names don't have to match.

Changes to method parameters will break source compat but only if the call-site is using named parameters, not sure how common that is but I find it unlikely here specially since AFAIK a virtual method like _input_event is only meant to be called by the engine and not users directly. Also, breaking source compat is not very disruptive anyway, so I think the changes here are fine.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Currently users get a "shadowing" warning for simply "doing the right thing". This should be fixed.

As mentioned above, compatibility is likely no problem.

While I would prefer event_position in CollisionObject3D to account for InputEventScreenDrag and InputEventScreenTouch, I guess mouse_position is also fine, because it covers the largest part of use-cases.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 27, 2024

I'd rather go with event_position as the other input events are also supported, so for me naming it "mouse position" feels like dumbing its usefulness down too much. Any possible ambiguity needs to be clarified on the docs side.

@akien-mga akien-mga merged commit 2ac8833 into godotengine:master Apr 4, 2024
16 checks passed
@KoBeWi KoBeWi deleted the position_who branch April 4, 2024 12:50
@akien-mga
Copy link
Member

Thanks!

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.

Change the "position" parameter in CollisionObject3D::input_event signal to avoid shadowing Node3D::position
7 participants