Handle errors while loading vector costumes or sounds #3637
Conversation
… broken costume data before finishing loading the costume.
…l error handling in one place. Could be used in the future to handle bitmap loading issues as well
… was not getting saved out properly.
kchadha
force-pushed
the
gqm-serialize-broken
branch
from
2c620d4
to
70a78cf
Compare
Co-authored-by: Christopher Willis-Ford <7019101+cwillisf@users.noreply.github.com>
…s that were missing when the project was first loaded
…04 page was being returned for missing asset data. Update VM to handle null assets properly.
…ge. Track additional metadata for broken costumes: 'bitmapResolution' and 'dataFormat'.
kchadha
changed the title
src/import/load-sound.js
Outdated
| @@ -61,6 +95,12 @@ const loadSound = function (sound, runtime, soundBank) { | |||
| runtime.storage.load(runtime.storage.AssetType.Sound, md5, ext) | |||
| ).then(soundAsset => { | |||
There was a problem hiding this comment.
Does this need a catch that ends up at handleSoundLoadError?
There was a problem hiding this comment.
Actually, thinking about this further: we should probably have tests that verify loadCostume and loadSound return default assets (with original data in .broken) if storage.load rejects. That might be out of scope for the GQM project, though, as long as what happens now is anything other than a data-loss-by-default-asset situation. (In this case I think it might be an unhandled exception => BSOD?)
There was a problem hiding this comment.
I decided to leave the error case out so as not to complicate this PR (which was supposed to be focused on vector costumes) too much more. This is similar to how I left off adding a .catch to loadBitmap_ for a later PR as well.
I think there should be a separate PR for broken bitmap costumes and a separate PR for corrupted sounds (or general sound error handling). I just wanted to make sure that we are handling storage.load resolving null in the same PR where we're bringing in the update to storage 2.0.1.
I am hoping to get those other 2 PRs in soon (barring complications I run into testing bitmaps..)
There was a problem hiding this comment.
Nvm! I will address corrupted sound stuff in this PR, since an error thrown here could result in the editor crashing, which might be more likely now that storage is swallowing less errors.
Also I assume you're talking about having unit tests for loadCostume and loadSound in addition to the integration tests? I might add those after this PR.
There was a problem hiding this comment.
Added 8ab21dd to address the sound error handling stuff
| obj.name = costume.name; | ||
| obj.bitmapResolution = costume.bitmapResolution; | ||
|
|
||
| const costumeToSerialize = costume.broken || costume; |
There was a problem hiding this comment.
yeah I had been itching to do this for a while but it didn't seem worth it for just 1 or 2 properties but now it's basically all of them! =D
Co-authored-by: Christopher Willis-Ford <7019101+cwillisf@users.noreply.github.com>
kchadha
changed the title
|
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 Wohooo!!! 🥳 Thanks for all your help with this work @cwillisf!!! The ambient (and real) pairing was a lot of fun and very much appreciated! ❤️ |
Resolves
Proposed Changes
While loading a vector costume, handle any errors in the process by displaying a gray question mark (same existing behavior) but with this PR, we now we keep track of the original costume data that failed to load.
When serializing the project out, we save the original (potentially broken) costume data instead of the default costume. This prevents data loss from the gray question mark costume overwriting potentially recoverable data (e.g. if the asset server was down when we first tried to load the costume).
Do not save any info about the costume being broken so that we make sure to try and load the costume again if we reload the project.
While the original costume contents are preserved, costume metadata may be fixed in the process of attempting to load the costume (e.g. if the data in the project.json like the costume md5 does not match the actual costume contents).
Update after review cycle and release of storage v2.0.0 & v2.0.1
This PR pulls in the fixes for these above issues via storage v2.0.1 and addresses handling storage now possibly returning null. This resulted in adding error handling while loading sounds as well. The same behavior described above about using the default sound but serializing what we know about the original sound is used here.
Reason for Changes
This fixes a critical issue that can lead to user data loss.
Test Coverage