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

Vendor extensions #103

Closed
LxLasso opened this issue Feb 4, 2020 · 11 comments
Closed

Vendor extensions #103

LxLasso opened this issue Feb 4, 2020 · 11 comments
Assignees

Comments

@LxLasso
Copy link

LxLasso commented Feb 4, 2020

Hope it hasn't been asked before, I didn't find anything when searching. We're about to launch the next version of Capture with cgltf and our own CAPTURE_model extension which has a very narrow target group. What's the lay of the land in terms of vendor extensions - manage them in our own branches, bring all into cgltf or introduce a structured way of hooking into cgltf to read / write them?

@jkuhlmann
Copy link
Owner

Thanks for asking!

To be honest, I'm not quite sure what the best way forward would be for vendor extensions, especially if they are niche. I don't want to bloat the code base with code that is only used by very few people. Do you see a way to make the implementation of your extension optional so that it's not part of cgltf.h directly? If not, maybe having it on a separate branch and referencing it from the master branch might be a way? Or do we need our own extension mechanism?
Are you going to submit the extension to Khronos as vendor extension?

@jkuhlmann jkuhlmann self-assigned this Feb 5, 2020
@LxLasso
Copy link
Author

LxLasso commented Feb 5, 2020

We are leaning towards submitting it yes, but I'm still trying to ascertain the best course of action.

At the moment it works fine to have our extension implementation in a separate branch, but maintaining it will eventually get old - especially if at any point we want to add some other vendor extension to the mix. Also it doesn't exactly help with sharing the implementation.

To be honest I would only expect KHR, EXT and widely used vendor extensions to go into cgltf so I would lean towards suggesting some form of extension mechanism.

It would be neat if one could simply include cgltf.h and then include say cgltf_CAPTURE_model.h etc. It could also serve as a staging mechanism for other "experimental" extensions like KHR_materials_clearcoat. Such extensions could live in the cgltf-repo, but it would be up to the consumer to choose which to include.

@jkuhlmann
Copy link
Owner

I agree that we should only have KHR, EXT and widely used extensions in cgltf by default. Do you already have a branch/repo with the current status of your implementation which I could take a look to figure out what an extension mechanism could look like?

@LxLasso
Copy link
Author

LxLasso commented Feb 6, 2020

Yes, it's in https://github.com/LxLasso/cgltf/tree/CAPTURE_fixture (the extension was originaly CAPTURE_fixture but I haven't updated the name of the branch). Note that only write is implemented at this point. The changes should all be tagged with CAPTURE_model start/end.

@LxLasso
Copy link
Author

LxLasso commented Feb 6, 2020

Also, here is the extension documentation in the event that it helps (a .md in a .zip)
README.zip

@zeitt
Copy link
Contributor

zeitt commented Apr 22, 2020

Since I currently need to be able to parse a couple vendor specific extensions, I made a pull request, which adds an "extensions" field to structures. That modification allows me to avoid reparsing the entire glTF json structure, and focus on small parts needed by extension.

Exposing extensions the same way extra data is exposed would have also worked, but I felt like directly exposing "dictionary" allows better access to specific extensions.

// pseudo example of custom parser I had in mind
struct translucent_material_ext{
    cgltf_material* main_material;
    cgltf_bool has_translucency;
    cgltf_texture* refraction_texture;
};
cgltf_result translucency_parser(const cgltf_data*, translucent_material_ext*)

//...
translucent_material_ext ext_material_array[data->materials_count];
cgltf_result res = translucency_parser(&data, &ext_material_array);

//...
cgltf_result translucency_parser(const cgltf_data* data, translucent_material_ext* out_materials) {
    for(int i = 0; i < data->materials_count; ++i)  {
        out_materials[i].main_material = &data->materials[i]
        out_materials[i].has_translucency = 0;
        for(int j = 0; j < data->materials[i].extensions_count; ++j)  {
            if(data->materials[i].extensions[i].name == "MY_translucency_ext") {                
                char json_str[1024];
                cgltf_size json_size;
                cgltf_copy_extension_json(data, &(data->materials[i].extensions[i]), &json_str, &json_size);
                
                json_obj json = json_obj_from_string(json_str);
                int source = json_get_int(&json, "source");
                if(source < 0) {
                    return cgltf_result_invalid_gltf;
                }
                out_materials[i].has_translucency = 1;
                out_materials[i].refraction_texture = &(data->textures[source]);
            }
        }
    }
    return cgltf_result_success;
}

@jkuhlmann
Copy link
Owner

@LxLasso PR #114 is merged now. Do you think it makes sense to implement support for vendor extensions on top of it?

@LxLasso
Copy link
Author

LxLasso commented Jun 19, 2020

Shiny! I've been following your progress in silence so far as I haven't had anything intelligent to add. :-P Will check it out right away!

@LxLasso
Copy link
Author

LxLasso commented Jun 19, 2020

It looks good!

I think it could have been handy to have a user pointer in cgltf_extension to have somewhere to store parsed data. It would then be easy to write some code that "expands" a specific extension into useable data for others. In our case we could then provide a function bool CAPTURE_model_parse_extension(cgltf_data* data, CAPTURE_model_context* context); that would read the JSON and set the user pointer to some parsed data structure. Info required to free any allocated memory could be managed within that context.

Next step would be to hook it into cgltf_write. Is that up for grabs are am I late to the game @prideout ?

@jkuhlmann
Copy link
Owner

I've got a new proposal in issues #125. Feedback would be appreciated.

@LxLasso
Copy link
Author

LxLasso commented Sep 20, 2020

With @zeitt 's addition and the discussion in #125 which suggests adding a user pointer in cgltf_extension I think we can close this issue.

@LxLasso LxLasso closed this as completed Sep 20, 2020
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

3 participants