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

Implement Smoothing Speed Control for Individual Axes in Camera2D #95717

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

VincentShao32
Copy link

The user can now control the position smoothing speed for the X and Y axes individually for smoothing in the Camera2D node, as opposed to only being able to control them together previously.

This was done by removing the previous position_smoothing_speed property and replacing it with two new properties: position_smoothing_horizontal_speed and position_smoothing_vertical speed. The smoothing calculation was altered to use these two values.

Here is what the new values in the editor look like.
image

Here's a video showcasing differing smoothing speeds on the axes.

Screen.Recording.2024-08-17.154200.mp4

I have verified that this works with limit smoothing as well.

@Calinou
Copy link
Member

Calinou commented Aug 17, 2024

This was done by removing the previous position_smoothing_speed property and replacing it with two new properties: position_smoothing_horizontal_speed and position_smoothing_vertical speed.

Removing a property (or renaming it) breaks compatibility with existing projects, so you must add two new properties and deprecate the old one instead. The old property can be hidden from the inspector if the value is equal to its default (define _validate_property() for that).

@VincentShao32 VincentShao32 requested review from a team as code owners August 18, 2024 02:06
@VincentShao32
Copy link
Author

This recent commit restored the value and should fix any compatibility errors.

@VincentShao32 VincentShao32 requested a review from a team as a code owner August 18, 2024 20:12
@VincentShao32
Copy link
Author

I decided to combine the two values I had before into one Vector2, making it possible to link the two values and also make it more intuitive for the user. The funcationality is the same. I also updated the .xml file for the Camera2D node.

Here is what the new UI looks like:
Screenshot 2024-08-18 130853

Here is a video demo:

Camera2D.Contribution.demo.mp4

@AThousandShips AThousandShips changed the title Implemented Smoothing Speed Control for Individual Axes in Camera2D Implement Smoothing Speed Control for Individual Axes in Camera2D Aug 18, 2024
@@ -26,7 +26,9 @@ def generate_bundle(target, source, env):
target_bin = lipo(bin_dir + "/" + prefix, env.extra_suffix + env.module_version_string)

# Assemble .app bundle and update version info.
app_dir = Dir("#bin/" + (prefix + env.extra_suffix + env.module_version_string).replace(".", "_") + ".app").abspath
app_dir = Dir(
"#bin/" + (prefix + env.extra_suffix + env.module_version_string).replace(".", "_") + ".app"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an entirely unrelated style change

Copy link
Author

Choose a reason for hiding this comment

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

Oh, that's strange since I didn't edit that file at all. I'm guessing pre-commit might have made some automatic edits. Should I change it back?

Copy link
Author

Choose a reason for hiding this comment

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

Pre-commit automatically changes it and I don't think I can bypass it.

Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -26,7 +26,9 @@ def generate_bundle(target, source, env):
target_bin = lipo(bin_dir + "/" + prefix, env.extra_suffix + env.module_version_string)

# Assemble .app bundle and update version info.
app_dir = Dir("#bin/" + (prefix + env.extra_suffix + env.module_version_string).replace(".", "_") + ".app").abspath
app_dir = Dir(
"#bin/" + (prefix + env.extra_suffix + env.module_version_string).replace(".", "_") + ".app"
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@AThousandShips AThousandShips removed the request for review from a team August 19, 2024 17:21
@AThousandShips AThousandShips removed the request for review from a team August 19, 2024 17:25
@AThousandShips
Copy link
Member

Please apply suggestions as a batch to avoid excessive commits

@VincentShao32
Copy link
Author

Ah, I didn't know you could do that. Yes I will do that in the future.

VincentShao32 and others added 10 commits August 21, 2024 08:12
The user can now control the smoothing speed for the X and Y axes individually for smoothing in the Camera2D node, as opposed to only being able to control them together previously.

This was done by removing the previous position_smoothing_speed property and replacing it with two new properties: position_smoothing_horizontal_speed and position_smoothing_vertical speed. The smoothing calculation was altered to use these two values.
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@VincentShao32 VincentShao32 requested review from a team as code owners August 21, 2024 15:16
@AThousandShips

This comment was marked as resolved.

@AThousandShips AThousandShips removed request for a team August 21, 2024 15:21
@VincentShao32 VincentShao32 force-pushed the camera-smoothing-axis-control branch from b56e5c2 to 4cb926c Compare August 21, 2024 15:24
@VincentShao32
Copy link
Author

Sorry, that was my fault I was just trying to update my branch. Do you know if/when my PR can be approved?

@AThousandShips
Copy link
Member

It needs feedback from the appropriate team, they have been requested

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Documentation is fine, if there was any doubt.

I can personally see the benefit, although it's a bit niche.
For more complex behaviours, writing your own code is not just necessary but encouraged.
"Rotation smoothing was accepted, so why not this?". I suppose one must argue where to draw the line.

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.

4 participants