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

Allow specifying fade length in beats/bars/seconds #99188

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timoschwarzer
Copy link
Contributor

@timoschwarzer timoschwarzer commented Nov 13, 2024

This PR allows to pick between beats/bars/seconds for AudioStreamInteractive transition lengths.

image

Todo

  • Backwards compatibility
  • Update docs

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch 2 times, most recently from b72ed61 to b8a2706 Compare November 13, 2024 22:05
@Calinou Calinou added this to the 4.x milestone Nov 13, 2024
@timoschwarzer
Copy link
Contributor Author

I changed the behavior of the options visibility and disabled state a bit to hide/disable controls that are not editing anything right now:

Screencast.From.2024-11-13.23-05-12.mp4

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch from b8a2706 to 84f2c78 Compare November 13, 2024 22:12
@timoschwarzer timoschwarzer marked this pull request as ready for review November 13, 2024 22:12
@timoschwarzer timoschwarzer requested review from a team as code owners November 13, 2024 22:12
@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch 2 times, most recently from 2cad74e to 19d2eb1 Compare November 13, 2024 22:42
@timoschwarzer timoschwarzer requested review from a team as code owners November 13, 2024 22:42
@RedMser

This comment was marked as resolved.

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch from 19d2eb1 to 396c550 Compare November 14, 2024 07:50
@timoschwarzer
Copy link
Contributor Author

@RedMser Helped a lot, thank you!

I hope my compat handling of the deprecated get_transition_fade_beats method is fine as it is now.

  • It will always return the fade length even if the unit is not "beats" anymore
  • This should be fine because when upgrading from an earlier version of Godot, everything has a unit of "beats" because nothing else existed before
  • When using the deprecated get_transition_fade_beats on a transition that has been set to use another unit than "beats" I print a warning that the return value is likely not what is expected from the (deprecated) function name and what methods to use instead.

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch from 396c550 to 3b3a0cb Compare November 14, 2024 08:02
@timoschwarzer
Copy link
Contributor Author

Some thoughts about renaming the method add_transition to add_transition_with_unit:

  • If we would want to keep the old name, the new parameter would need to go to the end of the parameter list. That would mean users need to provide values for use_filler_clip and filler_clip only to provide the unit. Also the unit would not be next to the value anymore:
    add_transition(0, 2, 0.0, 0.0, FADE_CROSS, 2.5, false, -1, UNIT_SECONDS)
    vs
    add_transition_with_unit(0, 2, 0.0, 0.0, FADE_CROSS, 2.5, UNIT_SECONDS)
  • The method could be renamed back to add_transition in the next major release because in the end the additional _with_unit suffix has no other reason than preserving compatibility.

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch from 3b3a0cb to d6d300a Compare November 14, 2024 16:16
@RedMser

This comment was marked as resolved.

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch 3 times, most recently from 8881a8b to 8220636 Compare November 14, 2024 16:27
@RedMser
Copy link
Contributor

RedMser commented Nov 14, 2024

Compat stuff looks good to me now, if the CI also does not complain 😉

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch from 8220636 to bdd1ccf Compare November 14, 2024 16:32
@timoschwarzer
Copy link
Contributor Author

@RedMser thanks a lot for the detailed explanation! 🙌 CI should be fine after the latest push 😄

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch 2 times, most recently from a838335 to 59a5083 Compare November 14, 2024 19:56
@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch from b2a432f to a6bbe09 Compare December 4, 2024 19:35
@timoschwarzer
Copy link
Contributor Author

Applied suggestions and squashed

@timoschwarzer timoschwarzer force-pushed the feature/audio-stream-interactive-fade-length-unit branch from a6bbe09 to ff0664a Compare December 6, 2024 07:58
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.

Allow fading by a fixed time in AudioStreamInteractive
4 participants