Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/cesium primitive outline #175

Merged
merged 17 commits into from
Mar 10, 2023

Conversation

bertt
Copy link
Contributor

@bertt bertt commented Feb 27, 2023

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
Copy link
Owner

vpenades commented Feb 27, 2023

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


public static class CesiumOutline
{
public static Accessor CreateCesiumOutlineAccessor(ModelRoot model, IReadOnlyList<uint> outlines)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I've also added ModelRoot as parameter (in addition to IReadOnlyList) because it makes creating the bufferView and Accessor more easy

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.... Given that Cesium has a bunch of extension, it's possible in the future other cesium related methods will be added, so I would rename "CesiumOutline" to "CesiumUtils" , "CesiumToolkit" "CesiumExtensions" , whatever you like more.

That way, all future cesium related methods can go in the same place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, renamed it to CesiumToolkit

@bertt
Copy link
Contributor Author

bertt commented Feb 28, 2023

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

/// </summary>
/// <param name="model"></param>
/// <returns>boolean, true is valid false is not valid</returns>
public static bool ValidateCesiumOutlineExtension(ModelRoot model)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a method that checks the indices of the used Cesium outline accessor against the mesh primitive indices. Todo: call this method from the glTF validator (if I know where to do this).

set => _indices = value;
}

protected override void OnValidateContent(Validation.ValidationContext validate)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added OnValidateContent method for indices validation

@vpenades
Copy link
Owner

vpenades commented Mar 1, 2023

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
Copy link
Contributor Author

bertt commented Mar 1, 2023

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
Copy link
Owner

vpenades commented Mar 1, 2023

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

@bertt
Copy link
Contributor Author

bertt commented Mar 7, 2023

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
Copy link
Contributor Author

bertt commented Mar 10, 2023

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 vpenades merged commit 50ba47c into vpenades:master Mar 10, 2023
@vpenades
Copy link
Owner

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants