fix crash due to undefined clipping planes texture #6576
Conversation
|
@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, so you can review this at any time. I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
Erm. I guess that's only "should fix" since I can't reproduce the specific behavior in that forum post/issue. So that's not great... but |
|
There was also a bug in here with partial texture updates for uint8 texture mode, and there wasn't a spec that caught it until I made a tweak to how double-texture allocation works. I'll add a more targeted test. Who wrote this... hmmmm... |
likangning93
force-pushed
the
fixClippingPlaneCrash
branch
from
c1e6c43
to
b5cd192
Compare
likangning93
force-pushed
the
fixClippingPlaneCrash
branch
from
b5cd192
to
7f45898
Compare
|
@lilleyse this is ready for review now. |
|
The code looks good and tests pass/demos work across browsers. |
|
Just a side note - I haven't been able to reproduce the bug myself but I asked the forum member who posted the bug to test it out if he is able. I'll be good to merge until then or otherwise within the next couple of days. |
|
@likangning93 there's a new error with the latest code:
|
| } | ||
|
|
||
| var pixelsNeeded = ClippingPlaneCollection.useFloatTexture(context) ? clippingPlaneCollection.length : clippingPlaneCollection.length * 2; | ||
| var requiredResolution = computeTextureResolution(pixelsNeeded, result); |
There was a problem hiding this comment.
From the error message it looks like this might be returning NaN because pixelsNeeded is 0.
Don't know how that's happening though, because the clipping plane shader regen shouldn't be called in that case. Wish we were able to reproduce the error for debugging... I'll keep trying to trace this in the meantime.
|
User on the forum provided a Sandcastle using the AGI HQ model and steps to reproduce:
|
|
The Sandcastle was replacing the tileset's existing clipping plane collection with a new one, but some individual Tiles were retaining references to the old clipping plane collection, which had been destroyed. |
|
Ah good, that means #6599 is related. |
likangning93
force-pushed
the
fixClippingPlaneCrash
branch
from
bca5a0d
to
8f97a0f
Compare
likangning93
force-pushed
the
fixClippingPlaneCrash
branch
from
8f97a0f
to
f2e3fc4
Compare
|
@lilleyse updated! |
| @@ -513,6 +513,12 @@ define([ | |||
| this._model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined; | |||
| } | |||
|
|
|||
| // If the model references a destroyed ClippingPlaneCollection due to the tileset's collection being replaced with a | |||
| // ClippingPlaneCollection that gives this tile the same clipping status, update the model to use the new ClippingPlaneCollection. | |||
| if (defined(tilesetClippingPlanes) && defined(this._model._clippingPlanes) && this._model._clippingPlanes.isDestroyed()) { | |||
There was a problem hiding this comment.
Alternatively could the check be tilesetClippingPlanes !== this._model._clippingPlanes?
Once #6599 is fixed we won't be destroying the collection anymore.
|
@lilleyse updated! |
| @@ -523,7 +523,7 @@ define([ | |||
|
|
|||
| // If the model references a destroyed ClippingPlaneCollection due to the tileset's collection being replaced with a | |||
There was a problem hiding this comment.
Fix comment here as well.
|
@lilleyse ready! |
|
Thanks! |
|
Congratulations on closing the issue! I found these Cesium forum links in the comments above: https://groups.google.com/forum/#!searchin/cesium-dev/mr If this issue affects any of these threads, please post a comment like the following:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Fixes #6566