Fixes unhandled rejection in Skybox.update #12306 #12307
Conversation
|
Thank you for the pull request, @angrycat9000! ✅ We can confirm we have a CLA on file for you. |
There was a problem hiding this comment.
Thanks for getting to the bottom of this @angrycat9000! A few comments about how we might fix it.
| that._cubeMap = that._cubeMap && that._cubeMap.destroy(); | ||
| that._cubeMap = cubeMap; | ||
| }, | ||
| function (error) { |
There was a problem hiding this comment.
Is there a particular reason to use the onRejected callback here? Otherwise we typically use catch. Even better would be to use async/await if possible.
There was a problem hiding this comment.
No, just habit. I can change to catch
I don't want to use await here because that would make the entire Skybox.update function asynchronous instead of just the loading code.
| that._cubeMap = cubeMap; | ||
| }, | ||
| function (error) { | ||
| console.error("Skybox cube map failed to load", error); |
There was a problem hiding this comment.
Typically, we don't like to log to the console, and prefer throwing errors where possible so the user can deal with it as appropriate for their app. Though the fact that this is in the update loop complicates things.
- Ideally, the async code would be removed from the update loop entirely, and there would be a top-level async function like
SkyBox.fromCubeMap()orSkyBox.fromImages(). But that may require more refactoring than you're willing to do at this time. - Next best would be to await the promise, catch the error, and throw it synchronously in the next call to
update. That way the unit tests can usepollToPromisecallupdateuntil an error is thrown or_cubeMapis defined. - Finally, as a comprise, could we instead use a
oneTimeWarning?
There was a problem hiding this comment.
2 is good. That preserves the expectation that the error is getting thrown, but allows the test to avoid throwing the error (as update is never called second time)
- Throw during next call to update - use catch instead of onRejected - Don't update CHANGES.md for minor bug
|
Thanks for the feedback @ggetz . Updated and ready for another look |
|
Test failure only occurs in release mode. For some reason |
The calls with the Thanks @angrycat9000, this all looks good! |
Description
Adds an error handler to
loadCubeMapinSkybox.updateand then throw any errors during the next call.This allows the error to be thrown in normal usage when
updateis called repeatedly. It prevents throwing the error after the end of a test becauseupdateis not called after the test ends. Previously the error would be thrown inafterAllwhen the tests were running.Issue number and link
Fixes #12306
Testing plan
npm run tests.git checkout main -- packages/engine/Source/Scene/SkyBox.jsto get the previous versionnpm run tests.afterAllAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change