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

ResourceImporterWAV: Allow configuring loop mode on import #59170

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Mar 15, 2022

The new edit/loop_mode import options lets user choose to either:

  • Detect loop points from the WAV (default, same behavior as before)
  • Set the loop mode and loop points manually like in AudioStreamSample

Previously the only options were to import loop points from WAV, or to force a full-length forward loop. There was no easy way to disable looping if it's embedded in the WAV, which confuses many users.

Fixes #46164, at least the most egregious part. The UX around editing loop points on an already imported WAV resource is still awkward.

Notes

  • For a potential 3.x backport, the compatibility should be preserved with the previous edit/loop boolean option. Could possibly be added in this PR too for master if wanted.
  • "Loop Begin" and "Loop End" import options are only shown when the "Loop Mode" is neither "Detect From WAV" nor "Disabled".
  • "Loop Begin" and "Loop End" import options support negative positions, so one can use -1 to select the end frame (and that's the default value for "Loop End").
  • I noticed that the generated spectrum preview in the Inspector actually reflects the loop mode when importing as "Backward" (so it's flipped), but that's not refreshed when reimporting. One needs to reopen the editor for the preview to match the import option at the time of opening.
simplescreenrecorder-2022-03-15_16.44.35.mp4

@akien-mga akien-mga added enhancement topic:audio topic:import cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Mar 15, 2022
@akien-mga akien-mga added this to the 4.0 milestone Mar 15, 2022
@akien-mga akien-mga requested a review from a team as a code owner March 15, 2022 15:12
The new `edit/loop_mode` import options lets user choose to either:
- Detect loop points from the WAV (default, same behavior as before)
- Set the loop mode and loop points manually like in AudioStreamSample

Fixes godotengine#46164.
@akien-mga akien-mga force-pushed the import-wav-configure-loop-mode branch from 210327d to b389ce5 Compare March 15, 2022 15:18
@akien-mga akien-mga requested a review from a team March 15, 2022 15:18
@reduz
Copy link
Member

reduz commented Mar 15, 2022

Now that we allow for advanced UI for some resoucre types, it should be possible to have an editor to show the audio file and edit the loop points.

@akien-mga
Copy link
Member Author

We approved this in PR review meeting, though there's room for improvement on the UX as @reduz suggested. He'll write a proposal about it so this can be improved further by any interested contributor.

@akien-mga akien-mga merged commit 99139e1 into godotengine:master Mar 22, 2022
@akien-mga akien-mga deleted the import-wav-configure-loop-mode branch March 22, 2022 12:26
@akien-mga
Copy link
Member Author

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 28, 2022
@EddieJr-DigiArts
Copy link

Toggling loop on and off should be part of editable settings together with pitch, volume, autoplay and such

@ellenhp
Copy link
Contributor

ellenhp commented Apr 28, 2022

Toggling loop on and off should be part of editable settings together with pitch, volume, autoplay and such

This is tricky because some AudioStreams cannot loop. AudioStreamMicrophone, AudioStreamGenerator, specifically.

We could add a loop property to an AudioStreamPlayer potentially, which would attempt to loop whenever the stream runs out of samples. This could have unintended consequences though because I believe play() is not always allocation-free, especially in the case of ogg/vorbis streams. I agree the current UX could be improved though. I'm just not sure how to do it.

@ellenhp
Copy link
Contributor

ellenhp commented Apr 28, 2022

If you have more thoughts on this though, there's an open proposal for it that we can continue discussion in godotengine/godot-proposals#3120

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.

WAV sound effect is looped although Loop is not checked
4 participants