vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
480 stars 76 forks source link

Read GLB with glTF extension #22

Closed bertt closed 5 years ago

bertt commented 5 years ago

I'm trying to read an GLB with 'CESIUM_RTC' extension (using ModelRoot.Load(f)), but got an InvalidDataException - The chunk must be padded to 4 bytes: 67554. See attachment for the GLB file.

Basically the extension adds an '_BATCHID' attribute to the vertices.

Is there any sample code how to read/write these files? with_batch.glb.zip

vpenades commented 5 years ago

Hi

I've checked your model with glTF validator and it gives the same error as SharpGLTF. Chunks must be aligned to 4 bytes length to be valid.

Technically speaking, I should do nothing, as per the glTF specification the model is invalid, and whoever wrote this GLB file should fix his code to write correct glTF files.

After all, we're all aiming to have a healthy glTF ecosystem with tools writing correct files, not an insane ecosystem with readers trying to fix others writers mishaps.

But we live in an imperfect world, and it's not the first time I find malformed glTF files that could probably be loaded, so it's a bit of a compromise.... my thoughts about this is to add an extra argument to bypass all the validation checks after loading a model.

By loading a model without the loading validation checks you would be open to unexpected behaviour all the way through the model parsing, so your code would need to be a lot more defensive when dealing with unchecked models, also, when something goes wrong, it would be difficult to tell if it's been because the model is malformed, or because an incorrect API usage.

Anyway, this feature is way down my priority list right now.

But even if the model didn't have the chunk misalignment, it would not load anyway since CESIUM_RTC is declared as a required extension, and SharpGLTF does not support that extension.

There's a discussion here whether this extension should be officially supported. Right now I'm only sticking to official extensions listed here , but if it's an extension that is commonly used, and not very hard to implement, I would consider adding it.

Btw, I can't add CESIUM_RTC extension unless the extension schema for glTF2 is published somewhere; SharpGLTF uses automatic code generation based of the schemas, so I need the CESIUM_RTC schema for glTF2 to generate the code from it.

bertt commented 5 years ago

In my case I only want to write the extra batch id for each vertice, see more info https://github.com/AnalyticalGraphicsInc/3d-tiles/blob/master/specification/TileFormats/Batched3DModel/README.md#binary-gltf

It seems to me the Cesium RTC extension does not cover this topic, I've got the impression its outdated (can only find a 1.0 schema).

Maybe there is a way to create valid glTF's and also add the _BATCHID attribute for each vertice? Or is there another way in glTF to indicate to which model a vertice belongs?

vpenades commented 5 years ago

Hmmm... if you want to write cesium compatible glTF files I think you'll need to do a lot more work... and I don't think SharpGLTF could do that out of the box right now.

To begin with, the Cesium_RTC is not supported... I believe that's a show stopper.

Then to write a custom vertex attribute with MeshBuilder, you would need to create your own vertex type (like a VertexPositionNormalBatchId or something like that), which MeshBuilder would accept and it could let you create MeshBuilders in that way.... but when writing the actual glTF, with the current pipeline, the attribute would probably be skipped.

Right now I'm struggling with morph targets support, I don't know how much time it will take.... afterwards I'll look at this issue.

bertt commented 5 years ago

ok thanks, I'm just investigating whats easily possible. In Cesium the models (like buildings) are grouped together in glTF's based on distance (for performance). The batchid is needed so the user can click/highlight an individual building to get more information about it.

bertt commented 5 years ago

I've created an GLB file with another tool (warning contains not valid glTF) which contains the batchid attribute but does not have RTC extension. It's a better example of what I want to be able to read/write. 1.glb.zip

vpenades commented 5 years ago

I've just commited some fixes, take a look at this test case

you will have to create your own vertex type, you can just copy VertexColor1Texture1Custom1 struct, and modify it to your needs.

bertt commented 5 years ago

one thing I noticed with this solution: VertexColor1Texture1Custom1 is calling FragmentPreprocessors but that class is inaccessible (internal by default)

vpenades commented 5 years ago

one thing I noticed with this solution: VertexColor1Texture1Custom1 is calling FragmentPreprocessors but that class is inaccessible (internal by default)

Ah, yes, that's true...

I could look at it to see if it's worth/useful to make it public...

But for custom attributes, FragmentPreprocessor has not way to tell if the custom values of the structure are fine or not...

So you have to implement your own checks... or simply leave it empty.

bertt commented 5 years ago

fyi: wrote little global tool to inspect the custom vertex attributes: https://github.com/bertt/gltf.tooling

bertt commented 5 years ago

a question: ideally I want to generate glb with only vertex attributes POSITION,NORMAL,_CUSTOM_1, the attributes COLOR_0 and TEXCOORD_0 are not needed. Is there a way to do this?

vpenades commented 5 years ago

Just create a struct implementing IVertexMaterial with the attributes you need.


    public struct VertexCustom1 : IVertexMaterial
    {
        public VertexCustom1 (Single customId)  { CustomId = customId;  }        

        public static implicit operator VertexCustom1(Single customId value)
        {
            return new VertexCustom1(value);
        }       

        public const string CUSTOMATTRIBUTENAME = "_CUSTOM_1";

        [VertexAttribute(CUSTOMATTRIBUTENAME, Schema2.EncodingType.FLOAT, false)]
        public Single CustomId;       

        public int MaxColors => 0;

        public int MaxTextCoords => 0;       

        public void SetColor(int setIndex, Vector4 color) { }

        public void SetTexCoord(int setIndex, Vector2 coord) { }

        public Vector4 GetColor(int index) { throw new ArgumentOutOfRangeException(nameof(index)); }

        public Vector2 GetTexCoord(int index) { throw new ArgumentOutOfRangeException(nameof(index)); }

        public void Validate() {  }

        public object GetCustomAttribute(string attributeName)
        {
            return attributeName == CUSTOMATTRIBUTENAME ? (Object)CustomId : null;
        }
    }
bertt commented 5 years ago

great, that works!

bertt commented 5 years ago

Think we can close this issue, adding a custom vertex attribute now works for me. Will there be a nuget alpha-0012 package with this functionality? At the moment I use a custom build package.

vpenades commented 5 years ago

Yes, we can close it up.

Creating custom vertex types has always been part of the Toolkit since the introduction of MeshBuilder... although it was not tested properly.

I'm still polishing a few details since I began working with morph targets building, which is one of the itchy issues I was not suporting yet, and also I found some issues that might be bugs and I want to confirm them first.... so expect a new package in about 1 or 2 weeks...

bertt commented 5 years ago

Ok great 👍