godotengine
Add runtime file loading to `AudioStreamWAV`
This PR adds a load_from_file method to AudioStreamWAV and ResourceImporterWAV. This method has the parameters path:String, and options:Dictionary.
This is helpful in the use-case where you want to load audio files at runtime (ex: a music player), and need to load a WAV file. These methods reuse the normal import functionality for WAV files to stay consistent with the rest of the engine, while allowing users to access this functionality in a convenient way.
This should fix godotengine/godot-proposals#732, although a slight refactor of WAV importing may be something to consider in the future.
Also: if there is a better way to provide the import parameters other than a Dictionary I would love to improve that for these new methods, I was just having issues trying to bind a static method using the HashMaps that are used when importing files normally, so I decided to switch it to a Dictionary for this specific function because of that. (I'm not the most knowledgeable on the Godot codebase currently, so I wasn't sure what else to do).
Also: if there is a better way to provide the import parameters other than a
DictionaryI would love to improve that for these new methods, I was just having issues trying to bind a static method using theHashMaps that are used when importing files normally, so I decided to switch it to aDictionaryfor this specific function because of that.
Because the passed parameters are identical to the contents of ResourceImporterWAV, why not just passing a singular ResourceImporterWAV object?
Also: if there is a better way to provide the import parameters other than a
DictionaryI would love to improve that for these new methods, I was just having issues trying to bind a static method using theHashMaps that are used when importing files normally, so I decided to switch it to aDictionaryfor this specific function because of that.Because the passed parameters are identical to the contents of ResourceImporterWAV, why not just passing a singular ResourceImporterWAV object?
Thanks for the idea, I don't know how I didn't think of that before 😅, will look into that ASAP!
After looking into this further it doesn't seem like I can use a ResourceImporterWAV object directly to pass around the import options, as the options were never stored in this class to begin with, if another better solution is found I'll look into it.
Okay, indeed. Then what may be worth doing for the time being is providing a codeblock example because just saying the properties are the same makes the functionality harder to discover.
Out of curiosity, why is this functionality exposed both in AudioStreamWAV and ResourceImporterWAV, especially since their arguments are the same? I'd suggest only exposing it in AudioStreamWAV, as ResourceImporter is not meant to be used outside of editor builds.
Okay, indeed. Then what may be worth doing for the time being is providing a
codeblockexample because just saying the properties are the same makes the functionality harder to discover.
Added a codeblock to the docs, it should be good for now I think. I decided against just relisting all the properties in a codeblock since the ResourceImporterWAV class is public and has proper descriptions for all the properties, but maybe I should? Not sure.
Out of curiosity, why is this functionality exposed both in AudioStreamWAV and ResourceImporterWAV, especially since their arguments are the same? I'd suggest only exposing it in AudioStreamWAV, as ResourceImporter is not meant to be used outside of editor builds.
I exposed it in both because I was using ResourceImporterOggVorbis as my main reference, and it also does that, but learning that normally you shouldn't (and looking at the currently failing builds other than editor) it seems like only exposing it in AudioStreamWAV is in fact the ideal solution.
I think now the main thing I'm looking at is getting this working outside editor builds, as currently it seems that (unsurprisingly) a file inside the editor/import folder isn't being compiled into non-editor templates. This can probably(?) be fixed by just relocating the file but I'm not sure if that's the most ideal solution in this case.
Instead of moving the files around I realized I could just move the logic for load_from_file to AudioStreamWAV since that should always be included in the engine when compiling, relying on it for imports now but retaining the same compatibility. This seems like it might've fixed a small issue in audio_effect_record.cpp where it was relying on the editor folder when it shouldn't have, so that seems good.
Any other feedback appreciated if any improvements can be thought of here (currently I think everything is pretty alright as-is for this feature though).
Added a load_from_buffer function to AudioStreamWAV as said before, everything seems to be working fine so far.
One thing to note: I fixed certain audio files not loading (and thus failing on unit tests) by removing a check for the file reaching eof "prematurely", which seems to be being triggered by a somewhat "false-positive" from FileAccessMemory I think?
Ever since adding load_from_buffer and using FileAccessMemory for that (which seems to work fine on the WAV files I used to test), the ones used for unit testing just wouldn't work with the check in place for whatever reason unless I removed the check.
I want to try getting this merged for Godot Engine Master 4.4.
Can we get a rebase on this?
@what-is-a-git Could you look into rebasing this PR to fix merge conflicts and applying the above suggestions? See PR workflow for instructions :slightly_smiling_face:
I'm working on it right now, gonna look through the suggested changes as well and do those.
Updated the commit to fix IMA ADPCM compression not being supported by AudioEffectRecord at runtime, since with the moving of the compression function it is now entirely possible to do it without referencing ResourceImporterWAV.
