Crash when setting an old clipping plane

[lilleyse]

The way we handle ownership for clipping planes might be too strict. Doing something like assigning a clipping plane to a tileset, assigning a different clipping plane, and then assigning the original clipping plane causes a crash. It might be better to leave the clipping plane deletion up to the user instead?

Sandcastle demo
In the demo uncheck the box and then check it again.

[likangning93]

We could also have the ClippingPlaneCollection just delete its GL resources (pretty much just the texture) when it gets detached so that the object itself is reusable. It'll just rebuild its resources the next time it gets updated.

Any objections?

[mramato]

So it's impossible to share clipping planes objects across primitives?

Collection properties should always be readonly. Settable collections in general is a bad API pattern.

[likangning93]

Settable collections in general is a bad API pattern.

@mramato is this just because of the under-the-hood GPU resource management?
Hmmmm I vaguely suspect moving these to readonly and auto-populating them on Model, Cesium3DTileset, and Globe would make life a little easier...

[mramato]

It's just problematic from an API endpoint (largely around ownership issues). A lot of Cesium initially followed the .NET coding guidelines, and they specifically call it out as bad practice: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/guidelines-for-collections

I'm sure Cesium is wildly inconsistent here, so leaving it settable is probably not the end of the world.

I would love for Cesium to be ref-counted anytime WebGL (or similar external) resources are involved, but I think that ship sailed a long time ago.

[hpinkos]

Here's your problem: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/ClippingPlaneCollection.js#L668

It destroys the old one when it's switched so you can't re-use it. Not sure what the right answer is here.

[mramato]

The way we handle this elsewhere is to have the user specifically tell us not to destroy stuff (and then it's on them to clean it up). PrimitiveCollection for example has an option called destroyPrimitives

This is one of the areas that the pure CS person in me wishes I could change about Cesium. We should really be using ref counting or something similar to better track usages. At some point in the future I would love to see a sweeping change in this regard, but we need to spend enough time thinking about it up front.