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

[BUGFIX] Fixed Polymod and Mods Support in FlxPartialSound Using OpenFL Assets #2

Merged

Conversation

KarimAkra
Copy link
Contributor

@KarimAkra KarimAkra commented Jun 9, 2024

we load the aseet's bytes with Assets.loadBytes (which works asynchronously) then create a haxe.io.BytesInput instance with the bytes retrieved instead of directly reading the file with FileSystem.read to get a FileInput for it

This should be merged with the change in this pr on Funkin

@ninjamuffin99
Copy link
Member

this looks yummy! I will mark this for myself for testing since it doesn't seem like it's too verbose of a workaround/fix! thanks!

@EliteMasterEric
Copy link
Member

@KarimAkra I'm not sure if your testing suggests otherwise, but it seems to me like this code is loading the whole file before cutting out a portion?

As opposed to the old code, where I believe the intent was to use the filesystem to read only a portion of the file directly from disk, this saving memory by never having the full file loaded at once.

@KarimAkra
Copy link
Contributor Author

KarimAkra commented Jun 9, 2024

@EliteMasterEric
I've just made a quick comparison between the build on itch.io and the one i compiled with these changes

Both seemed to be using the same amount of memory that's varying between 220-230 mb
And it adds up around 5-10 mb on both builds every time a portion plays

So i think it's safe to say this dosent have any impact on the memory usage & optimization!

@KarimAkra
Copy link
Contributor Author

Turns out Bytes don't get cached or allocated to memory unless they're pushed to the openfl cache map or something like that
So yes, i can confirm this dosen't impact memory usage at all

@ninjamuffin99
Copy link
Member

hmmm it doesn't save the full audio file to memory that does seem to be correct, but one thing about this is that it does at least try to load the whole audio file right? like Assets.loadBytes(path) i assume would attempt to load the full file (even if it doesn't affect memory), and wouldn't complete the little event listener until the full file has been loaded? in which case I think something closer to the original implementation is intended, so that we ONLY load exactly the bytes we want out of the audio file, so that it hopefully loads up quicker.

i havent used fileinput and file reading too much to be honest! my implementation could be doing the same thing! but the intention is to get it to read ONLY the slice of bytes it needs/wants for a preview

@KarimAkra
Copy link
Contributor Author

KarimAkra commented Jun 10, 2024

That is actually true now that you mentioned it
But to see the difference and determine the impact on load speed, i made a quick benchmark to compare the load time of FileInput and BytesInput (with loading the whole file bytes)

image

here are the results

image

Given these results, the impact on speed when using BytesInput instead of FileInput is minimal. Therefore, it should be safe to use BytesInput without significant performance concerns.

@KarimAkra
Copy link
Contributor Author

if you're wondering what's the audio file i used in here

it's the 2hot instrument (1.50 mb)

Copy link
Contributor

@moondroidcoder moondroidcoder left a comment

Choose a reason for hiding this comment

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

After testing it on my own laptop, the impact isnt really that big, even spamming to load an hour long podcast audio file doesnt impact performance. Results, laptop specs, and the audio file are all below.
Mod testing seems to go well though.

image

image

Audio file

@EliteMasterEric
Copy link
Member

@KarimAkra Can you revert your most recent change? Assets.loadBytes is SUPPOSED to work here, I need to fix things on the Polymod side.

@KarimAkra
Copy link
Contributor Author

@KarimAkra Can you revert your most recent change? Assets.loadBytes is SUPPOSED to work here, I need to fix things on the Polymod side.

I looked deep into how Assets.loadBytes works before making these changes

It uses lime.utils.Bytes.fromFile (https://github.com/openfl/lime/blob/develop/src%2Flime%2F_internal%2Fbackend%2Fnative%2FNativeHTTPRequest.hx#L421-L427)

Bytes.fromFile dosen't use the lime/openfl library assets tree it's like sys.io.File.getBytes so it can't really work with polymod afaik

https://github.com/openfl/lime/blob/develop/src%2Flime%2Futils%2FBytes.hx#L94-L98

@EliteMasterEric
Copy link
Member

EliteMasterEric commented Jun 17, 2024

Bytes.fromFile dosen't use the lime/openfl library assets tree it's like sys.io.File.getBytes so it can't really work with polymod afaik

Before it reaches that call, Lime calls AssetLibrary.loadBytes(), which gets overridden by Polymod. It just isn't implemented properly.

https://github.com/larsiusprime/polymod/blob/6d1bdf79b463ca0baa8471dfc6873ab7701c46ee/polymod/backends/LimeBackend.hx#L626-L642

@KarimAkra
Copy link
Contributor Author

Bytes.fromFile dosen't use the lime/openfl library assets tree it's like sys.io.File.getBytes so it can't really work with polymod afaik

Before it reaches that call, Lime calls AssetLibrary.loadBytes(), which gets overridden by Polymod. It just isn't implemented properly.

https://github.com/larsiusprime/polymod/blob/6d1bdf79b463ca0baa8471dfc6873ab7701c46ee/polymod/backends/LimeBackend.hx#L626-L642

Reverted it

I think Polymod needs to use a complete alternative method for loadBytes instead of calling the one from the lime AssetLibrary

I don't really know much abt how Polymod functions so I guess you'll have figure this out

@EliteMasterEric
Copy link
Member

I got it working on the latest develop commit of Polymod. It currently has a SLIGHT delay on the first play but otherwise seems to work well even on ZIP mods.

@EliteMasterEric EliteMasterEric self-assigned this Jun 18, 2024
@EliteMasterEric EliteMasterEric merged commit a1eab7b into FunkinCrew:main Jun 18, 2024
@KarimAkra KarimAkra deleted the openfl/polymod-implement branch June 18, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants