vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
454 stars 72 forks source link

Feature/cesium primitive outline #175

Closed bertt closed 1 year ago

bertt commented 1 year ago

This pull request adds support for glTF 2.0 extension 'CESIUM_primitive_outline', specs see https://github.com/KhronosGroup/glTF/tree/main/extensions/2.0/Vendor/CESIUM_primitive_outline

In the unit test method 'CreateCesiumOutlineTriangleScene' outlines are added to a simple triangle, result in Cesium looks like (notice the black outlines):

image

vpenades commented 1 year ago

Overall, the PR looks good!

I would change these, though:

        public void SetCesiumOutline(int? indices)
        {
            if (indices == null) { RemoveExtensions<CESIUM_primitive_outlineglTFprimitiveextension>(); return; }
            var ext = UseExtension<CESIUM_primitive_outlineglTFprimitiveextension>();
            ext.Indices = indices;
        }

is too low level to me, because the 'indices' argument is very ambiguous. Given it's the index of an accessor, at the very last I would change it to:

        public void SetCesiumOutline(Accessor indices)
        {
            if (indices == null) { RemoveExtensions<CESIUM_primitive_outlineglTFprimitiveextension>(); return; }

            // do proper Accessor checks, like being owned by the same model as this primitive, and the data type is INT, etc.

            var ext = UseExtension<CESIUM_primitive_outlineglTFprimitiveextension>();
            ext.Indices = indices.LogicalIndex;
        }

As a bonus, I would overlad the validation to check that if the extension exists, all the indices fall within the available indices of the primtive. That way, on validation, it would give an exception if there's invalid indices

Another bonus would be to have an easy to call CreateCesiumOutline(IReadOnlyList indices) that would create and fill the accesor

bertt commented 1 year ago

I've added the CreateCesiumOutLine method and checks, the validation I will look into next

vpenades commented 1 year ago

The PR looks fine to me, if you don't plan to add anything else I'll merge it.

I'll probably publish new packages to also address the missing lights and cameras.

bertt commented 1 year ago

ok nice, at the moment I'm testing with a larger dataset. Looks like the extension works fine but there is something going wrong with getting the outline indices list, I'll look into it.

image

vpenades commented 1 year ago

Ok, I'll wait until you tell me everything's addressed.

bertt commented 1 year ago

update: got a live demo working looks good so far. Still checking on Draco compression and tileset validation. https://bertt.github.io/cesium_3dtiles_samples/samples/outline/delaware/

bertt commented 1 year ago

I've made a second demo https://bertt.github.io/cesium_3dtiles_samples/samples/outline/utrecht/, the extension works well it's ready for merge.

vpenades commented 1 year ago

@bertt you might want to take a look at the changes I've made

I fixed the class name generation, and I've simplified the API, the CesiumToolkit class is no longer needed... look at the test to see how to use now (easier, I think)