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 CollisionObject3D parameter shadowing #78864

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Jun 30, 2023

resolve godotengine/godot-proposals#5715

The signal input_event parameter position shadows the property Node3D.position.
For a better usability, change the parameter name as suggested in that proposal.

Updated 2023-07-05: Virtual function _input_event has the same problem. Fix it also for that function.

@Sauermann Sauermann requested review from a team as code owners June 30, 2023 04:43
@akien-mga akien-mga added this to the 4.2 milestone Jun 30, 2023
The signal `input_event` parameter `position` shadows the property
`Node3D.position`. Same goes for the virtual function `_input_event`.
For a better usability, change the parameter name.
@Sauermann Sauermann force-pushed the fix-collision-parameter-collision branch from 5f147d5 to 814d1e8 Compare July 5, 2023 16:38
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Nov 1, 2023
@AThousandShips AThousandShips requested a review from a team March 21, 2024 11:08
@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)

@raulsntos
Copy link
Member

This changes the parameter name of a signal and a virtual method.

  • Changing parameter names of signals doesn't break compat in C#.
    We generate a delegate for the C# event, when used like this users can't invoke it so it won't break compat.
    To subscribe to these events, the lambda parameters can use whatever name the user wants.
  • Changing parameter names of virtual methods doesn't break compat in C# for implementers.
    The override can use different parameter names, although it's recommended to use the same ones as the original declaration.
  • Changing parameter names of methods breaks source compat in C# for callers if the call-site was using named parameters (e.g: MyMethod(position: myPosition)).
    But I'd say this is uncommon for these virtual methods that are only meant to be called by the engine, and source compat is not a very disruptive break so I think it's OK to break compat here.

@Sauermann
Copy link
Contributor Author

Superseded by #89721

@Sauermann Sauermann closed this Apr 4, 2024
@Sauermann Sauermann deleted the fix-collision-parameter-collision branch April 4, 2024 12:48
@akien-mga akien-mga removed this from the 4.3 milestone Apr 4, 2024
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
4 participants