Refactor audio (pre)loading

[arthuro555]

The current way of loading audio has a problem. There is no real preloading, so when audio is played there is a slight delay (the time for the audio file to be actually loaded). This is because we use one new Howl instance every time we want to play a sound.

This PR has for goal to cache the Howl instance of every file and reuse them. That way it is also possible to add to audio resources an attribute to let the user preload and cache the Howl instance on the loading screen.

This PR also makes HowlerSound use Composition over Inheritance, something that is preferred for this codebase if I understood correctly (Though currently it is done in a dirty way).

There is one downside though, as the Howl instances are cached, it can make the game take some more memory as they never get properly unloaded. This could be fixed with an action to clear the cache and to clear one file/resource from the cache.

[arthuro555]

Ok, now it is ready for proper reviewing.

[arthuro555]

My main concern is about running multiple times a sound, will this work? I'm afraid the Howl object would be shared so only one sound at a time of the same file would work?

Well, I assumed so siince there is the whole IDs system, and that there is this as an example in the docs:

If not though then I have a problem because the whole PR relies on that concept 😅

[4ian]

Well, I assumed so siince there is the whole IDs system

Ah nice, I think I did not understand how it works! If that's the case that's even better then :) I re-read the documentation and it seems that it's properly handled so no worries.

[4ian]

Sounds better than the old system then 👍👍

[4ian]

Very excited about these changes :) Let's triple check that all conditions/actions are working, in particular with multiple sounds and loading of preloaded sound/musics/not preloaded. Maybe a quick test game to make for this (or find one that would already exist, but I'm not sure)?

[arthuro555]

I began to try adding typescript typecheking, sadly it doesn't support well overloaded functions :/ I added the chaining of HowlerSound methods to be consistent with howler. I marked setRate/getRate as deprecated as both rate and setRate/getRate were used in the code, and I think duplicated APIs are bad, so I kept the one consistent with the other APIs. This shouldn't impact most users, as long as they don't use it via JS. I fixed all the points you mentioned.

[4ian]

I added the chaining of HowlerSound methods to be consistent with howler.

Let me know what you think, but as explained in the comment I'm not a huge fan of these magic functions, they are impossible to statically type and probably a nightmare for JIT. Not sure why people like them, writing getXxx/setXxx is not really long nor verbose and very clear.

Anyway, let me know what you think we should do. Keeping compatibility with Howler is interesting, but on the other hand in the future if we replace it, we'll have this kind of "fluent" api that does not look like anything else in the codebase to maintain.

[arthuro555]

I think overloading is indeed not a very good idea. As we have autocompletion in the IDE and API docs (though I believe their generation is currently broken due to bad typing in gd.js), it shouldn't be really a problem for the users to find the syntax for GDevelop. In the worst sceneario, they can use the public createHowlerSound interface to get access to a raw Howl if they want to.

Do you think I should also remove the chaining for setters? I think that a fluent api is better when mass-setting properties of a sound.

[4ian]

In the worst sceneario, they can use the public createHowlerSound interface to get access to a raw Howl if they want to

Yep. Or we could add an accessor to the howler sound if really needed one day (let's not do it now).

Totally ok for the fluent setters 👍

[arthuro555]

Ok, I think all that's left is testing. Silver gave me a project for testing, let's try it out :)

[arthuro555]

Ok, everything seems to work properly, I think the next step would be to let people try it out in their game to see if there is a breaking change as this is a massive rewrite of the sound system, and then it should be ready to merge I think.

[4ian]

Alright I think we've reached the point where we can now do a few days of testing with maybe a testing version for people to check that it works on their game :) I'll also test on iOS on Sunday.

[arthuro555]

Apparently, according to testers, this is very broken. It's weird because the reported bugs seem to have appeared for things I have already tested and didn't modify (like music between scenes or volume). It's definitely not merge ready.

[4ian]

Could be related to events? Maybe the easiest way to debug is to add some console.log a bit everywhere to see what is being played/paused/stopped/etc. Should allow to quickly see if the behavior is correct or not.

[arthuro555]

Ok I fixed two major issues. The first one was that my unloading actions didn't delete HowlerSounds using the unloaded Howl, so it resulted in semi unloaded zombie sounds with very weird behavior. My second one was completely messing up the listeners by using the onload unnecessarily, and using normal functions instead of arrow functions making this not working properly.

There are still some other problems I need to fix though.

I think the listeners are not a really good idea the more I think about it, because they sound like a memory leak. As JS doesn't has destructors, we cannot remove the listeners. The listeners have a function that encloses this, and is stored in the Howl which is still accessible as it is stored in the sound manager, so a mark and sweep garbage collector will not collect the HowlerSound until it's associated Howl is collected too. Then maybe I am wrong, I am not the most experienced JavaScript user 😅 .

[4ian]

The first one was that my unloading actions didn't delete HowlerSounds using the unloaded Howl, so it resulted in semi unloaded zombie sounds with very weird behavior.

Good catch. When you launch an audio, it will continue to exist in memory even if you drop all references to it. This is because they are not garbage collected, they continue to exist in the browser memory - where JS objects that have no reference to them are garbage collected and removed from memory.

garbage collector will not collect the HowlerSound until it's associated Howl is collected too.

Indeed, if you drop all references to HowlerSound in the sound manager, it's still possible that the HowlerSound is living in memory as long as the associated Howl exists, because the associated Howl has listeners, so we have this "chain":

HowlerSound -> listener function -> this pointing to HowlerSound.

This may mean that we need to clean the listeners in a destroy function in gdjs.HowlerSound. This was probably not needed before because dropping all references to gdjs.HowlerSound also meant dropping the only reference to the Howl. (in this destroy function, we call off on all listeners for the id)

[arthuro555]

This may mean that we need to clean the listeners in a destroy function in gdjs.HowlerSound. This was probably not needed before because dropping all references to gdjs.HowlerSound also meant dropping the only reference to the Howl. (in this destroy function, we call off on all listeners for the id)

I am not a fan of this, If a user uses a gdjs.HowlerSound in JS events, it won't necessarily be managed by the Sound Manager and the destroy method could never get called. I think the public API should be memory leak free, and those listeners are redundant and only meant as a failsafe. Maybe we should just directly remove them?

[4ian]

I am not a fan of this, If a user uses a gdjs.HowlerSound in JS events, it won't necessarily be managed by the Sound Manager and the destroy method could never get called. I think the public API should be memory leak free

I agree it's less convenient, but that's also why an API is not necessarily public: we don't always want user to create gdjs.HowlerSound on their own. That's our choice to make this a public api or not. If we consider it's too risky because it's an implementation detail, we can just ask people to use the gdjs.SoundManager. And that's what they should already do :)

The API is leak free if we create a destroy method. Again it's surely less convenient to use and more error prone, but it's maybe the only solution.

As for why listeners were needed, I remember using an action to launch a sound on a channel, then using the condition to check if the sound was stopped (this can be useful to do something once the sound is finished). Problem: the sound was not immediately considered as playing by Howler, so the condition would be directly true.

I'm ok to remove these listeners if we ensure this use case works correctly. Note that we can do this by choosing that a sound that is somehow "loading" is considered as playing (because it will play once the loading is done).

Otherwise, we can still keep the listeners if we properly ensure that we call off on them. It was not needed before because of not sharing Howl objects. It's now probably needed, so better do it, even if it's at the cost of adding a few calls to a destroy() function. I'm ok with this because we can consider our HowlerSound as an implementation details that people should not use directly, marking it private. (and this is a good example of why exposing too much APIs as public can sometimes be harmful for the future :))

[4ian]

Maybe we should just directly remove them?

Give it a try if you wish, but it will be important to try to keep the guarantee that when you launch an action to play a sound, the sound is immediately after considered by GDevelop as:

• playing, even if has a short loading before being actually launched.
• stopped only when it reach the end or if it can't be played.

Which make me think.... maybe we don't need gdjs.HowerlSound at all! :) After all, it was just an abstraction to guarantee what I just wrote before (and also to clamp the rate, but it's something that is easy to do). Maybe we could directly use and store Howls, and be sure that when we check if a sound is played, we check that either it's playing or it's loading. Would need careful tests, but maybe this could be a great simplification of this whole stuff... Give it a thought!

[arthuro555]

I think HowlerSounds should most definitely be a public API. Controlling a sound is a very basic thing, if we close up too much the API, people that use JS (and according to what I see on the forum and discord, the number is quite significant) will be forced to be using the event tools which give a very bad developer experience. The howler sound is therefore necessary to document all of that. Granted, it should not be documented as a Howler sound, but through a ISound interface, as there are other implementations possible (like cocos sounds).

[arthuro555]

Something I just realized is that this is a breaking change for JavaScript devs, as the overloaded methods on HowlerSound have been replaced with the getter/setter ones!

[4ian]

Don't worry about backward compatibility for now, we can always add later adapter from old functions to the new get/set ones. Let's first ensure we have a 100% correct/not leaking implementation.

[arthuro555]

I converted this PR to typescript, now I think the only thing left to do was to fix this bug: https://forum.gdevelop-app.com/t/gdevelop-music-improvements-testing/28256/11?u=arthuro555

[arthuro555]

🤔 It seems typescript isn't recognizing gdjs.SoundManager anymore. Anyone knows what I might have done wrong?

[arthuro555]

Ok I fixed the bug that was reported on the forum back then. I posted a new build on the forum to let people hopefully test it out. All that is left to do now is fix the weird typescript error where it doesn't find gdjs.SoundManager anymore and a bit of typo fixing.

[4ian]

All that is left to do now is fix the weird typescript error where it doesn't find gdjs.SoundManager anymore and a bit of typo fixing.

For some reason Semaphore CI is not triggering, can you post the full error here?

[arthuro555]

$ tsc
Runtime/runtimegame.ts:87:37 - error TS2551: Property 'SoundManager' does not exist on type 'typeof gdjs'. Did you mean 'JsonManager'?

87       this._soundManager = new gdjs.SoundManager(
                                       ~~~~~~~~~~~~

  Runtime/jsonmanager.ts:27:16
    27   export class JsonManager {
                      ~~~~~~~~~~~
    'JsonManager' is declared here.

Runtime/runtimegame.ts:151:29 - error TS2694: Namespace 'gdjs' has no exported member 'HowlerSoundManager'.

151     getSoundManager(): gdjs.HowlerSoundManager {
                                ~~~~~~~~~~~~~~~~~~

Runtime/runtimescene.ts:981:29 - error TS2724: 'gdjs' has no exported member named 'SoundManager'. Did you mean 'JsonManager'?

981     getSoundManager(): gdjs.SoundManager {
                                ~~~~~~~~~~~~

Found 3 errors.

error Command failed with exit code 2.

[4ian]

I see! Where in the code do you assign gdjs.SoundManger to gdjs.HowlerSoundManager? Look at how it's done for Pixi renderers to get an idea (at the end of each file).

[arthuro555]

I didn't change it: https://github.com/4ian/GDevelop/pull/2006/files#diff-4ffbb004738df5211810d820b417a55bb34023e6df14b029303bdef385bf21bcR731

[4ian]

Seems like the import type { ... } from '...' is making the namespace "ignored" (see the opacity on gdjs):

Not sure why but surely because this kind of "global" namespace is a specific TypeScript thing.

[4ian]

To avoid losing too much time on this, you can translate all the exports to declare in Howler.d.ts Not as nice, but it will work for now?

[arthuro555]

Indeed, it worked. Thanks, I would have never guessed this was the cause.

[arthuro555]

According to this forum user who found other issues, the PR works fine now

[4ian]

Great news! I'm a bit behind in terms of reviews between this, the array/boolean variables and BitmapText, but I'll take a look :)

[arthuro555]

No worries, take your time.

[4ian]

Can you check the "Game Feel demo"? When firing enough time at various targets, at some point the sound effects are not working anymore. The background music effect is still played, but no more fire/impact.

I've tried multiple times and it seems that it's fine when you fire on nothing, but as soon as you start firing enough on different targets, there will be multiple sound effects, and at some point it seems that one stops working entirely (fire sound), then quickly the rest are also stopping.

[arthuro555]

Indeed, i can reproduce this issue too. I didn't manage to find where the issue could lie yet. What bothers me the most is that there isn't any kind of error/warning Message in the console.

[4ian]

We may be leaking audio resources in one way or the other (like stopped sounds are never cleared), so when we try to play a new one, it's not launched because there already are too much created? Is it possible that it's triggering because we're playing a sound while another is played, so the Howl ids are confused or something like this?

[4ian]

I tried looking at the Howler global object with this version (left) and the old version (right):

(this is after firing enough rounds to stuck the sound in the left version)

I don't see any problem at first sight on the new version. It's actually better as expected because there is only 3 howls (one for the background music, one for the fire effect and one for the impact SFX), rather than one for each sound that was played (this is actually a memory leak/bad management of howls in the current version!).

My hypothesis is that somehow the howls are stuck and when you try to play them it's not doing anything.

[4ian]

If I put a breakpoint in the playSound method of a HowlerSoundManager, and log this:

Not sure if this is related, but HowlerSound _id has a mix of undefined and null. Looks suspicious?

EDIT: And none of these are considered stopped:

EDIT2: Because of undefined/null, trying to call getSeek will sometimes return the Howl itself:

So there is at least definitely two problems:

• sounds that are ended are never cleaned, and never considered stopped (which used to be the case before, so this has to be fixed).
• id are set to undefined/null for some reason.

[4ian]

Might be useful to understand what's wrong too, a Howl in my case has 6 _onplay functions:

which seems to be due to the once('play', () => {. Looks suspicious too?

[arthuro555]

This looks like an issue with howler. I tried console logging the new ID and the old one:

Howler is responsible for generating those IDs, so I guess the issue should be coming from there.

I think it is related to this (I replaced howler with the non minified version for this debugging session): It seems that it recycles an old Sound, keeps its seek, and as it is above the end of the sound it return undefined and stop the sound :/

[4ian]

Great investigation.. might be worth then asking on the Howler GitHub if these lines are correct - at least null should be returned to be consistent with the beginning of the method where null is returned if the "sprite" passed is not found.

But even with null, they should return if the "seek" is at the end and an id is provided (i.e: if you call play(id) with an id of something that reached the end, you don't need to play anything). BUT if an id is not provided, this means you need to start a new sound. So if you recycle an existing sound, the seek should be set back to 0.

Do you want to try to open an issue on Howler.js GitHub to ask about this? The logic seems flawed. Looking at the "blame", it seems that these lines are 7 years old - so possibly a bug that was never noticed if people never used a lot the "id" capabilities of Howler.js :)

[arthuro555]

Ok so I found a "fix" (more like a hack): In howler, replace this line: https://github.com/goldfire/howler.js/blob/7c50da154af52bd4971ae75acbf6c078d256cd12/src/howler.core.js#L820 With var seek = 0;. This is a hack that breaks sprite sounds, but we don't use it anyways. I opened an issue on the howler repo to report that.

There is still an issue though, stopped() indeed doesn't seem to work. I wonder if it is related to the recycling of Sound instances done by howler which make us "lose" the data associated to sounds that ended?

[4ian]

Yeah your workaround is also valid as long as we're not using sprites :)

I think any usage of sound.something before calling setParams here is risky in this "play" method implementation:

I imagine sound._rate could also be out of date if you run a sound with no id, and then it recycle an old sound for which the rate was changed.

[arthuro555]

I wonder if it would be worth it to try and make a Howler fork that

1. Removes sprites as they are not used in GDevelop and cause such weird behavior
2. Removes the overloaded function to use setAttribute/getAttribute methods instead

The disadvantage of course is that this is more code to maintain and that it would be harder to apply updates to Howler, but it would allow us to fix such issues by ourselves, and to strip the stuff we do not need in GDevelop to make it more lightweight.

[4ian]

Mmm digging a bit more, it seems that when you get a recycled sound, it's properly reset so seek should be 0. https://github.com/goldfire/howler.js/blob/7c50da154af52bd4971ae75acbf6c078d256cd12/src/howler.core.js#L2044-L2048

Can you log the seek to verify if it's always 0? :)

[4ian]

I would avoid a fork at all cost for now. Howler did not cause any bugs for years, sprites here are not the cause of the bug (they introduce more complexity in their codebase, but here it seems not linked) and it's not worth saving 1kb, that is nothing compared to a single image of a game, by paying multiple hours of update in their codebase to remove a feature, then spend multiple hours applying their updates and commits to our fork in the next 3 years, then see that we've actually introduced 2 bugs, and got an answer "well you did your own fork so are you sure the bug you report is not in your codebase" the next time we report one ;)

[arthuro555]

Can you log the seek to verify if it's always 0? :)

I wonder... Those numbers look a lot like an ID, don't them?

Mmm digging a bit more, it seems that when you get a recycled sound, it's properly reset so seek should be 0.

That's using getInnactiveSound, right? But I think there is this weird thing that reuses a sound without resetting it if there is only one non ended sound: https://github.com/goldfire/howler.js/blob/master/src/howler.core.js#L750-L766

[4ian]

But I think there is this weird thing that reuses a sound without resetting it if there is only one non ended sound:

Ah nice finding, I'm not entirely sure what this is doing, but seems like it's reusing a song if there is only one that is paused.. A bit weird, and might be buggy or not, but I think a bug here would likely result in "a sound is played not from the beginning but from where I last paused a previous sound". But it would trigger only when a sound is paused, so likely not the problem here?