Kode / Kha

Ultra-portable, high performance, open source multimedia framework.
http://kha.tech
zlib License
1.48k stars 171 forks source link

[Krom/HL] Implement asset loading failed callbacks for most cases #1441

Closed MoritzBrueckner closed 1 year ago

MoritzBrueckner commented 1 year ago

This PR implements the failed callback on Krom and HL targets in most situations. This means that the "Could not read hxBytes of undefined" errors are finally gone in most cases – they didn't tell anyone what asset caused the problem and they were difficult to debug from remote if some Armory users for example simply shared just that error message. More importantly, it can no longer happen on Krom or HL that Image objects are returned to the user that neither contain a texture or a render target, thus crashing the application at a later point in time.

A few remarks:

I didn't implement the failed callback in HL for loading videos because I wasn't sure how to handle things in kinc_video_init() (e.g. video.cpp.h for Windows) which doesn't have a return value at the moment and on Windows simply ignores the HRESULT values returned from the functions it calls. I don't know if you want to slightly change Kinc's API, but I think this is required in order to get error information out of this method. Preferably there would be some more stuff in place that in case of an error would print the actual reason to the console (e.g. the HRESULT value if != S_OK). I think this would have a very minor impact on performance but would be an tremendously helpful change.

The same holds true for loading sounds:

As for loading images in HL, I changed Image.fromFile() so that it returns null in the error case. If you don't want to do this, I can instead move that code into the LoaderImpl, but that would be a rather hacky workaround and I think the current way is better since users will notice any issues before the application fails in mysterious ways at a later point in time. It probably makes sense to change the return type of Image.fromFile() to Null<Image> to make the new behaviour clear and to satisfy Haxe's null safety checks, but it would introduce more inconsistency for the Kha API since it's not really used anywhere else in Kha, so I'm not sure whether you'd want that.

Loading non-existing sounds still crashes on Kode/Krom. Fixing this is pretty simple (see https://github.com/armory3d/armorcore/pull/56), but I don't want to open PRs for untested code. Please see below on why I can't test for Kode/Krom.

The current state of things when attempting to load files that don't exist (✔ meaning the failed callback is called): Krom HL
Images
Sounds ✔ (Armorcore), ❌ crash (Kode/Krom) ❌ crash
Blobs
Fonts
Videos ❌ crash

Please note that I could test my Krom-related changes only with Armorcore and not with Kode/Krom, since the latter fails to build with a linker error due to kickstart() being disabled by an #if 0 block:

1>   Creating library [...]\Krom\build\x64\Debug\Krom.lib and object [...]\Krom\build\x64\Debug\Krom.exp
1>windowsunit.obj : error LNK2019: unresolved external symbol kickstart referenced in function WinMain
1>[...]\Krom\build\x64\Debug\Krom.exe : fatal error LNK1120: 1 unresolved externals
RobDangerous commented 1 year ago

Please add Kinc-issues for what you need. The Audio-API needs some changes anyway.