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

Read GLB with glTF extension #22

Closed
bertt opened this issue Aug 26, 2019 · 15 comments
Closed

Read GLB with glTF extension #22

bertt opened this issue Aug 26, 2019 · 15 comments

Comments

@bertt
Copy link
Contributor

bertt commented Aug 26, 2019

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

vpenades commented Aug 26, 2019

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

bertt commented Aug 26, 2019

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

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

bertt commented Aug 26, 2019

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

bertt commented Aug 26, 2019

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

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

bertt commented Aug 27, 2019

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

@vpenades
Copy link
Owner

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

bertt commented Aug 28, 2019

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

@bertt
Copy link
Contributor Author

bertt commented Aug 29, 2019

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

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

bertt commented Aug 29, 2019

great, that works!

@bertt
Copy link
Contributor Author

bertt commented Aug 29, 2019

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

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

bertt commented Aug 29, 2019

Ok great 👍

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

No branches or pull requests

2 participants