Vector tiles draping on 3d tiles (polygon & polyline)

[danielzhong]

Description

new api:

new Cesium.BufferPolygonCollection({
  heightReference: Cesium.HeightReference.CLAMP_TO_3D_TILE,
});
scene.primitives.add(collection

CLAMP_TO_TERRAIN: drape on terrain 
CLAMP_TO_3D_TILE: drape on 3d tiles
CLAMP_TO_GROUND: drape on both
none(default): drape on nothing

Issue number and link

https://github.com/iTwin/cesiumjs-web3d-internal/issues/29

Testing plan

Polygon

Polyline

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

AI acknowledgment

• I used AI to generate content in this PR
• If yes, I have reviewed the AI-generated content before submitting

If yes, I used the following Tools(s) and/or Service(s):
Github Copilot

If yes, I used the following Model(s):
Claud Opus 5

[github-actions]

Thank you for the pull request, @danielzhong!

✅ We can confirm we have a CLA on file for you.

[mzschwartz5]

Currently reviewing - one comment up front: could we get a test sandcastle that demos a real (or more realistic) dataset? Preferably one that might stress test this feature a little?

The other two sandcastle look pretty good!

edit - also, here's a sandcastle I had claude generate to see if draping works on other ellipsoids. This is on Mars (which is a 3d tileset). It indicated that the polygon may be misplaced because the Cesium3DTileset class (and related classes) bake in a hardcoded WGS84 assumption in a few places. I can't tell if it's placed correctly or not, but at least it drapes correctly!

[danielzhong]

Currently reviewing - one comment up front: could we get a test sandcastle that demos a real (or more realistic) dataset? Preferably one that might stress test this feature a little?

The other two sandcastle look pretty good!

edit - also, here's a sandcastle I had claude generate to see if draping works on other ellipsoids. This is on Mars (which is a 3d tileset). It indicated that the polygon may be misplaced because the Cesium3DTileset class (and related classes) bake in a hardcoded WGS84 assumption in a few places. I can't tell if it's placed correctly or not, but at least it drapes correctly!

Thanks for testing this! I don't think the vector drape path hardcodes WGS84 anywhere. Maybe Clipping is affected though.

I asked AI to create another sandcastle

[mzschwartz5]

I don't have the DC national parks data set so I'm getting a 404 error on that sandcastle. Re: wgs84, I was thinking about some locations in Cesium3DTile and Model3DTileContent that it gets hardcoded, but they seem to be unrelated, I think.

[mzschwartz5]

First pass looks pretty good! Mix of some questions, style comments, and just a couple bigger points on design and/or technical concerns.

[mzschwartz5]

Didn't we have a discussion about this with polyline draping on terrain? In the end we decided it would be better (at least for now) to just have the consumer set show = false on the collection

[donmccurdy]

The scene parameter is missing from this parameter list. Unfortunately I don't think tsd-jsdoc allows us to inherit the types from BufferPrimitiveCollection automatically.

I've included a fix in #13711 because it broke type checking otherwise, but probably best to add the parameter JSDoc in this PR. :)

[donmccurdy]

One sandcastle is no longer rendering vectors for me, and the other is no longer draped, something may have changed? No errors shown.

[donmccurdy]

Possibly this._heightReference isn't needed here because it's accessible on the collection?

[mzschwartz5]

I'm pretty opposed to the tight coupling here between BufferPrimitiveCollection and VectorProvider. In my mental model, BufferPrimitiveCollection is supposed to be a very low-level concept. Here, it's depending on a much higher level concept (VectorProvider).

There are a handful of ways this could be addressed. Probably the creator or owner of the collection should be invoking the updating / baking of the collection. I'd have to think some more.

Similarly, I still feel that (though it's a little less obvious) that heightReference shouldn't be a property of this class.

@donmccurdy thoughts?

[donmccurdy]

I agree that the BufferPrimitiveCollection→VectorProvider dependency is probably not ideal. I'm less sure what that should be replaced with, though. Ultimately BufferPrimitiveCollection is a renderable primitive (maybe it shouldn't be?) and I would like users to be able to drape vector collections created programmatically (not just when loading 3D Tiles). I'm not sure the VectorProvider API should be public yet, and finalizing a public API for VectorProvider API is probably too large a change before the Sept 1 release.

That could mean either a public VectorProvider API, or keeping VectorProvider internal and adding a "wrapper" around BufferPrimitiveCollection for rendering/styling purposes, something like...

const collection = new Cesium.BufferPointCollection();
const circleLayer = new Cesium.CircleLayer({collection, ...});
const groundCircleLayer = new Cesium.GroundCircleLayer({collection, ...});
const billboardLayer = new Cesium.BillboardLayer({collection, ...});
const labelLayer = new Cesium.LabelLayer({collection, ...});
scene.primitives.add(circleLayer, billboardLayer);

... which could be valuable if we want to match the styling features of classic CesiumJS APIs on the higher-performance BufferPrimitiveCollection data model ... but now I'm drifting way out of scope for this review. :)

tl;dr — open decisions to be made, beyond the scope of this PR, but I don't want to block this PR on it.

[danielzhong]

I made a issue to track this, for future refactor

[donmccurdy]

Now that BufferPrimitiveCollection is calling markForBaking internally when .collection.update(...) runs, does it still make sense to call it again here?

[mzschwartz5]

Per my other comment, I think it makes more sense to call it here and let BufferPrimitiveCollection be entirely unaware of VectorProvider

[donmccurdy]

Just theoretically (not suggesting you actually change anything!) ... would this be equivalent to precomputing a vertex attribute, containing the UV coordinates for each vertex relative to the model's own rectangle?

I am still trying to understand the new shader code here. 😓

[mzschwartz5]

Yes, theoretically, it should be the same. And that's what globe/terrain tiles give us. But we have full control over the vertex attributes of globe and terrain. We don't have the same control for models/3d-tiles.

(Well, nothing is stopping us from adding a vertex attribute to users' models/tilesets . But for heavy models, that could be a lot of extra data and processing time to precompute the attribute. The trade off here is that we push the burden onto the vertex stage, every frame. Might warrant performance testing).

[mzschwartz5]

Also, if there's a way to clarify what's going on here, please suggest! When I refactored the code into a common shader helper (originally from Jeshurun's voxel work), I personally found it quite complex. It took me some good time to wrap my head around, so I tried to comment heavily to make it easier for future readers. But it's hard to step out of my own head and get a clear view when doing so.

[danielzhong]

If the model is draped then model._scene must exist, do we need the model._vectorDataProvider property as well?

The retained reference is mainly needed for the release path, not the acquire path. scene.vectorProvider is basically scene.globe?.vectorProvider. When we get to the point where _vectorData needs to be released, that provider may no longer be accessible from the scene. For example, the globe may have been removed or replaced, and destroy() could happen afterward.

That’s why the model keeps a reference to the provider that originally allocated the data. If we try to get it back through model._scene?.vectorProvider during cleanup, we could either fail to release the data or end up releasing it through a different provider instance. In either case, the baked textures could be leaked or released incorrectly.