-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Srwiseman 6272 fix #6305
Srwiseman 6272 fix #6305
Conversation
Welcome to the Cesium community @srwiseman! Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Thanks @srwiseman. We received your CLA. @lilleyse can you review this when available? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @srwiseman.
@@ -327,6 +327,12 @@ define([ | |||
function onModelError(error) { | |||
console.error(error); | |||
} | |||
function checkModelLoad(model, entity, modelHash){ | |||
model.readyPromise.otherwise(function(error){ | |||
onModelError(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine now to just copy the contents of onModelError
into here and remove that function.
@@ -137,9 +134,12 @@ define([ | |||
url : resource.url, | |||
animationsRunning : false, | |||
nodeTransformationsScratch : {}, | |||
originalNodeMatrixHash : {} | |||
originalNodeMatrixHash : {}, | |||
loadFail: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small style tweak, add a space before the :
loadFail : false
Also add the fix to |
Thanks! If I have time, I'll make these changes this evening. |
@lilleyse Done! |
Thanks @srwiseman. |
@lilleyse is there a reason we merged this without tests? |
Ah... my bad. Yes there should be a test. @srwiseman would you like to add one, or leave that to us? |
Hi @lilleyse, sorry I was on vacation with no internet for a while. Did you write a test for this yet? If not, I could write one. |
Ah no problem. I haven't written a test, so you are still welcome. |
Cool. So basically the test would check to ensure that if the model fails to load, the bounding sphere state correctly gets updated to FAILED? Correct? |
Yes that sounds good. |
Fixes #6272.
This was a pretty basic fix, but it was causing me some issues since
model.readyPromise
evaluates within a loop. Therefore, if I tried to create a function like.otherwise(function() {...});
, eslint would fail with 'no-loop-func'.To solve this, I created a function
checkModelLoad()
. If this function determines the the model will not load, it sets a flag calledloadFail
within the model data. This flag is then checked when theboundingSphereState
is evaluated.