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

Expose AudioStreamMP3 loop properties to the editor inspector #55570

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

DeleteSystem32
Copy link
Contributor

@DeleteSystem32 DeleteSystem32 commented Dec 3, 2021

Exposes loop and loop_offset properties of AudioStreamMP3 resources to the editor inspector to mirror behaviour of other AudioStream resources. Addresses #53855.

I'm not 100% sure this should be both an editor property and an import setting, what's the general consensus on this?

Bugsquad edit: Fix #53855

@DeleteSystem32 DeleteSystem32 requested a review from a team as a code owner December 3, 2021 02:22
@ellenhp
Copy link
Contributor

ellenhp commented Dec 3, 2021

What's the use-case for this? The only thing I can think of is that it could be useful for making many copies of the same audio resource without making many copies of the same file (e.g. by opening the audio stream in the inspector, making edits, then doing a save-as operation to persist those changes in a tres). Does that sound about right or is there more we can do with this?

@Nukiloco
Copy link

Nukiloco commented Dec 3, 2021

What's the use-case for this? The only thing I can think of is that it could be useful for making many copies of the same audio resource without making many copies of the same file (e.g. by opening the audio stream in the inspector, making edits, then doing a save-as operation to persist those changes in a tres). Does that sound about right or is there more we can do with this?

In the editor properties, you can set the loop and loop_offset for ogg type of audio however currently you can't set the loop and loop_offset for mp3 type of audio.

@Chaosus Chaosus added this to the 4.0 milestone Dec 3, 2021
@DeleteSystem32
Copy link
Contributor Author

What's the use-case for this? The only thing I can think of is that it could be useful for making many copies of the same audio resource without making many copies of the same file (e.g. by opening the audio stream in the inspector, making edits, then doing a save-as operation to persist those changes in a tres). Does that sound about right or is there more we can do with this?

In the editor properties, you can set the loop and loop_offset for ogg type of audio however currently you can't set the loop and loop_offset for mp3 type of audio.

This is the main reason. AudioStreamMP3 is the only AudioStream type that doesn't have these properties exposed, so this is mainly for consistency and to avoid confusion. To add, not having to re-import just to change the loop and loop_offset properties might save some people some time.

@ellenhp
Copy link
Contributor

ellenhp commented Dec 3, 2021

I don't agree that this will avoid confusion, but I'm approving it anyway because I don't like that MP3s are different from the rest of the streams right now. If anything I think it's pretty confusing that these properties will now show up in the inspector and the import options screen, but that's just a quirk of the way Godot works. Thanks for finding this and bringing the mp3 stream to parity!

@akien-mga
Copy link
Member

Yes it's a source of confusion for users that needs to be addressed more generally for all audio formats. It's good to make MP3 match the current status quo indeed.

@akien-mga akien-mga merged commit 0e26152 into godotengine:master Dec 3, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.4 labels Dec 3, 2021
@akien-mga
Copy link
Member

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 Dec 6, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.4.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.

MP3 properties not in Inspector
5 participants