CesiumGS / cesium-native

Apache License 2.0
402 stars 205 forks source link

Add new CesiumQuantizedMeshTerrain library/namespace #842

Closed kring closed 4 months ago

kring commented 4 months ago

This is a PR into #828 so merge that first.

The main motivation for moving the quantized-mesh and layer.json functionality it to its own library is that this makes it easier to use the same code generator we use for glTF and 3D Tiles to generate classes for reading/writing layer.json.

So, this new library / namespace has:

Other related bonus changes:

csciguy8 commented 4 months ago

The term "legacy terrain" here refers to quantized-mesh and layer.json.

Kind of a pre-review question, and maybe naïve, but any reason we need to call this format "legacy"?

For example:

Previous experiences have made me a little biased against using the word "legacy" in code bases, but am open to it if it has use.

kring commented 4 months ago

Well, I think CesiumTerrain is way too general. Terrain is increasingly represented using 3D Tiles. IMO it's very much fair to call quantized-mesh and layer.json a legacy format at this point, though that doesn't mean it will go away tomorrow. Anyone who has a strong opinion on this, please chime in. If we don't like the term Legacy, I guess maybe I'd call it CesiumQuantizedMeshTerrain? Though that's not quite right either because this library would also include the even more legacy heightmap-1.0 format if we ever feel the need to support it.

csciguy8 commented 4 months ago

Anyone who has a strong opinion on this, please chime in.

I don't have a strong enough opinion to push too hard. More like, if I can convince you with the reasons, great. If not, let's move on.

(take these musings with a grain of salt, just a personal take)

csciguy8 commented 4 months ago

Hey @kring , I'm getting an error generating the gltf classes.

Following the instructions from here

Running npm run generate-3d-tiles and npm run generate-legacy-terrain seems to work fine, but running npm run generate-gltf gives me the error below.

Any ideas?

image

pjcozzi commented 4 months ago

@kring @csciguy8 I would suggest to avoid "legacy" in the name as Cesium is still actively supporting and producing data in quantized-mesh.

kring commented 4 months ago

I'm getting an error generating the gltf classes.

This was caused by a change in the glTF schemas. It was fixed in this PR: https://github.com/CesiumGS/cesium-native/pull/851

So we just need to merge in main.

I would suggest to avoid "legacy" in the name as Cesium is still actively supporting and producing data in quantized-mesh.

Thanks @pjcozzi.

Maybe CesiumOGTerrain then? 😆 In all seriousness, I think CesiumQuantizedMeshTerain is the best alternative, so I'll rename it to that.

pjcozzi commented 4 months ago

Maybe CesiumOGTerrain then? 😆 In all seriousness, I think CesiumQuantizedMeshTerain is the best alternative, so I'll rename it to that.

@kring lol. 👍

kring commented 4 months ago

@csciguy8 I've merged in main and renamed the library / namespace. This is ready for another look.

kring commented 4 months ago

Your changes look good to me. Thanks @csciguy8!