Skip to content

Conversation

ACrazyTown
Copy link
Contributor

@ACrazyTown ACrazyTown commented Sep 14, 2025

Closes #3466.

  • Deprecates loadEmbedded(), loadStream() and loadByteArray() and replaces them with load() and loadFromURL().
  • Deprecate FlxG.sound.stream() for FlxG.sound.loadFromURL() to stay consistent
  • Add loadStreamed() to allow loading streamed sounds. Additionally adds required functionality to FlxG.assets Will be addressed in a separate PR
  • Modifies FlxSoundAsset to allow passing in a ByteArray.

@Geokureli
Copy link
Member

Geokureli commented Sep 17, 2025

Is this something that can be unit tested? I'm genuinely not sure.

@ACrazyTown
Copy link
Contributor Author

I'm a bit unfamiliar when it comes to unit tests. Maybe we could check if the internal sound (_sound) is created and not null to see if Flixel managed to detect the data properly. I'm not sure if we can check if the sound loaded properly though, maybe checking length?

@Geokureli
Copy link
Member

NVM, FlxSoundTest doesn't actually test any of the existing args, so let's look into that in the future

@Geokureli
Copy link
Member

Just gonna say it, a byte array isn't "embedded", so it's odd to use this to load it... Maybe we should add a new load() method and deprecate loadEmbedded (maybe in a later version)?

Thoughts?

@ACrazyTown
Copy link
Contributor Author

Just gonna say it, a byte array isn't "embedded", so it's odd to use this to load it... Maybe we should add a new load() method and deprecate loadEmbedded (maybe in a later version)?

Thoughts?

Sounds good. While we're at it, I think it might be worth renaming loadStream() as well. From what I can tell the main purpose of it seems to be to load sounds from web locations, but there's a different concept of "streamed sounds" that would be nice to have built in.

Streamed sounds are helpful to use less memory when dealing with large music tracks for example. Instead of loading the entire file and playing it, they load (and unload) smaller chunks of data as they play.

We could have something like load(), loadFromURL() and loadStreamed() (but maybe that could be an argument in load() instead)

@Geokureli
Copy link
Member

loadFromURL seems best to me

Also Openfl does have the ability to "stream" audio, but I don't know if flixel leverages this

@ACrazyTown
Copy link
Contributor Author

Also Openfl does have the ability to "stream" audio, but I don't know if flixel leverages this

I believe it doesn't. I think we just have to use Assets.getMusic() (which internally sets up the sound to be streamed) instead of Assets.getSound()

@Geokureli
Copy link
Member

Also Openfl does have the ability to "stream" audio, but I don't know if flixel leverages this

I believe it doesn't. I think we just have to use Assets.getMusic() (which internally sets up the sound to be streamed) instead of Assets.getSound()

I forgot how I did it, but there's a way on html5 to stream a sound from the web

@ACrazyTown
Copy link
Contributor Author

Should I repurpose this and also include the load() changes here or should that be done in a seperate PR?

@Geokureli
Copy link
Member

Should I repurpose this and also include the load() changes here or should that be done in a seperate PR?

Please include that change, here

@ACrazyTown ACrazyTown marked this pull request as draft September 18, 2025 15:29
@ACrazyTown ACrazyTown changed the title Allow passing in ByteArray to FlxSoundAsset Rework FlxSound load methods Sep 18, 2025
@ACrazyTown ACrazyTown marked this pull request as ready for review September 18, 2025 22:26
@ACrazyTown ACrazyTown marked this pull request as draft October 6, 2025 09:11
@ACrazyTown
Copy link
Contributor Author

ACrazyTown commented Oct 6, 2025

There's still some quirks I need to figure out related to audio streaming. To not bloat this PR I'll strip out parts related to audio streaming and re-add them later in a separate PR.

@Raltyro
Copy link

Raltyro commented Oct 6, 2025

I misread this as rework flxsound like entirely, because i've did reworked the flxsounds once in one of gulp... FNF engine, which is still a mess that i was thinking of making a pull request on it later when its stable

@Raltyro
Copy link

Raltyro commented Oct 6, 2025

Maybe don't restrict MUSIC asset type to lime_vorbis in the future? since who knows if lime is going to add another format being able to streamed beside vorbis

@ACrazyTown
Copy link
Contributor Author

ACrazyTown commented Oct 6, 2025

Maybe don't restrict MUSIC asset type to lime_vorbis in the future? since who knows if lime is going to add another format being able to streamed beside vorbis

Yeah, that's something that could be addressed in a patch/minor release with a compiler conditional. I have to stick to checking lime_vorbis and messing with the VorbisFile API currently because Lime doesn't really have a proper cross-platform way to check if audio streaming is supported.

@ACrazyTown ACrazyTown marked this pull request as ready for review October 6, 2025 16:21
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 FlxSoundAsset from ByteArray
3 participants