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 Camera2D position smoothing properties not being grouped #74251

Merged
merged 1 commit into from
Mar 5, 2023

Conversation

MarcusElg
Copy link
Contributor

@MarcusElg MarcusElg commented Mar 2, 2023

#65779 introduced a bug where Camera2D position smoothing properties aren't grouped due to the property names not matching the group name. This pr fixes that.

Before:
image

After:
image

EDIT: Also fixed the 3to4 project upgrader upgrading to invalid follow_smoothing property names

@MarcusElg MarcusElg requested a review from a team as a code owner March 2, 2023 19:58
@kleonc
Copy link
Member

kleonc commented Mar 3, 2023

Also it's inconsistent that position smoothing speed is shown even though position smoothing is disabled, whilst this isn't the case for rotation smoothing. However, that is a seperate issue and is related to godotengine/godot-proposals#4823

Actually the issue is in here:
https://github.com/godotengine/godot/blob/9ea7fb57bfba7828b026bf7d8bb92c999612a084/scene/2d/camera_2d.cpp#L702-L704
"smoothing_speed" should be changed to "position_smoothing_speed".

Also seems strange the internal variables are still named follow_smoothing_enabled, etc. Likely it was overlooked in #65779.

@MarcusElg
Copy link
Contributor Author

Also it's inconsistent that position smoothing speed is shown even though position smoothing is disabled, whilst this isn't the case for rotation smoothing. However, that is a seperate issue and is related to godotengine/godot-proposals#4823

Actually the issue is in here:

https://github.com/godotengine/godot/blob/9ea7fb57bfba7828b026bf7d8bb92c999612a084/scene/2d/camera_2d.cpp#L702-L704

"smoothing_speed" should be changed to "position_smoothing_speed".
Also seems strange the internal variables are still named follow_smoothing_enabled, etc. Likely it was overlooked in #65779.

The internal variables can be renamed without breaking any compatibility right?

@kleonc
Copy link
Member

kleonc commented Mar 3, 2023

The internal variables can be renamed without breaking any compatibility right?

private ones yeah. But follow_smoothing_enabled is protected so I wonder if theoretically it could be used from a module/GDExtension when extending Camera2D. But not sure about this. Also not sure about the policy regarding these, maybe they are not guaranteed to be unchanged, I don't know. Maybe @akien-mga could enlighten us a little on that. 🌞

@akien-mga
Copy link
Member

GDExtension only has access to the same methods as GDScript (bound in _bind_methods).
As for C++ module compatibility, there's no guarantees. It's the price of having access to everything, module developers deal with our private/internal APIs. Compatibility is only aimed for the scripting API/bindings.

@MarcusElg MarcusElg force-pushed the positiongroup branch 3 times, most recently from 9b54039 to 5214cd3 Compare March 3, 2023 17:55
@MarcusElg
Copy link
Contributor Author

I fixed the issues mentioned in the discussion, the internal properties has been renamed to match the exported names. However I noticed that the 3to4 project converter converts to old incorrect names such as "follow_smoothing_enabled". Should we change those as well?

@kleonc
Copy link
Member

kleonc commented Mar 3, 2023

However I noticed that the 3to4 project converter converts to old incorrect names such as "follow_smoothing_enabled". Should we change those as well?

Yeah, as it's incorrect anyway. Currently it would rename smoothing_enabled to non existing follow_smoothing_enabled property (another overlooked thing in #65779).

{ "smoothing_enabled", "follow_smoothing_enabled" }, // Camera2D


Note there are also some in C# renaming map:

{ "GetFollowSmoothing", "GetFollowSmoothingSpeed" }, // Camera2D

{ "SetEnableFollowSmoothing", "SetFollowSmoothingEnabled" }, // Camera2D

{ "SetFollowSmoothing", "SetFollowSmoothingSpeed" }, // Camera2D

{ "SmoothingEnabled", "FollowSmoothingEnabled" }, // Camera2D
{ "SmoothingSpeed", "FollowSmoothingSpeed" }, // Camera2D

cc @raulsntos Just to be sure: all of these are also incorrect and the new names should have FollowSmoothing changed to PositionSmoothing, right?

@MarcusElg
Copy link
Contributor Author

However I noticed that the 3to4 project converter converts to old incorrect names such as "follow_smoothing_enabled". Should we change those as well?

Yeah, as it's incorrect anyway. Currently it would rename smoothing_enabled to non existing follow_smoothing_enabled property (another overlooked thing in #65779).

{ "smoothing_enabled", "follow_smoothing_enabled" }, // Camera2D

Note there are also some in C# renaming map:

{ "GetFollowSmoothing", "GetFollowSmoothingSpeed" }, // Camera2D

{ "SetEnableFollowSmoothing", "SetFollowSmoothingEnabled" }, // Camera2D

{ "SetFollowSmoothing", "SetFollowSmoothingSpeed" }, // Camera2D

{ "SmoothingEnabled", "FollowSmoothingEnabled" }, // Camera2D
{ "SmoothingSpeed", "FollowSmoothingSpeed" }, // Camera2D

cc @raulsntos Just to be sure: all of these are also incorrect and the new names should have FollowSmoothing changed to PositionSmoothing, right?

Ok just as I expected, I've pushed a fix.

@raulsntos
Copy link
Member

Just to be sure: all of these are also incorrect and the new names should have FollowSmoothing changed to PositionSmoothing, right?

Yes, but keep in mind that the methods are internal in 4.0 and the properties should be used instead. I don't think the project converter is able to do this kind of conversion but I also don't expect these methods to have a high usage since they were marked obsolete in 3.x.

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

LGTM.

What @raulsntos mentioned regarding how C# methods which were changed to be internal in 4.0 are treated by the 3to4 converter seems like a potential seperate issue, out of scope of this PR.

@akien-mga akien-mga merged commit b7c0200 into godotengine:master Mar 5, 2023
@akien-mga
Copy link
Member

Thanks!

@MarcusElg MarcusElg deleted the positiongroup branch March 5, 2023 12:50
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.1.

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.

None yet

5 participants