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

Support for custom extensions #231

Closed
alteous opened this issue Jul 21, 2019 · 17 comments · Fixed by #395
Closed

Support for custom extensions #231

alteous opened this issue Jul 21, 2019 · 17 comments · Fixed by #395

Comments

@alteous
Copy link
Member

alteous commented Jul 21, 2019

There are two formally supported extensions by the crate: KHR_materials_pbrSpecularGlossiness and KHR_lights_punctual. As discussed in #226 there is no way to retrieve any other extension data. I believe it's now possible to provide unrecognized extensions alongside the recognized ones using a combination of #[serde(flatten)] and serde::value::RawValue.

{
    "extensions": {
        "KHR_lights_punctual": {},
        "EXT_some_other_extension": {}
    }
}
#[derive(Deserialize, Serialize)]
pub struct Root {
    #[serde(default, serialize_only_if = "Option::is_none")]
    pub extensions: extensions::Root,
}

pub mod extensions {
    #[derive(Deserialize, Serialize)]
    pub struct Root {
        #[serde(default, rename = "KHR_lights_punctual", serialize_only_if = "Option::is_none")]
        pub khr_lights_punctual: KhrLightsPunctual,
        #[serde(default, flatten)]
        pub others: HashMap<String, RawValue>,
    }
}

As for retrieving the data, for now I propose a simple getter:

pub mod extensions {
    pub struct Root<'a> {
        document: &'a Document,
        json: &'a json::extensions::Root,
    }

    impl<'a> Root<'a> {
        pub fn others(&self) -> &HashMap<String, RawValue> {
            &self.others
        }
    }
}
@zellski
Copy link

zellski commented Aug 8, 2019

Likely naive question: If this feature is implemented as suggested above, would it be reasonably easy for a user to fish out (hypothetical example) root.others["KHR_texture_transform"] and, if non-empty, perform a follow-up pass of serde-deserialisation on?

Having access to the raw JSON is vastly better than not having it, but it'd be even better if I could hook up my own KhrLightsPunctual-like struct hierarchy.

@alteous
Copy link
Member Author

alteous commented Aug 9, 2019

Yes, that's the idea. I imagine user code would look something like this:

if let Some(raw_value) = root.others().get("KHR_texture_transform") {
    #[derive(Deserialize)]
    struct KhrTextureTransform {
        pub foo: i32,
        ...
    }
    let khr_texture_transform: KhrTextureTransform = serde_json::from_str(raw_value.get())?;
}

@3c1u
Copy link

3c1u commented Sep 7, 2019

I'd prefer that the extension can be deserialized directly into struct.

#[derive(..., Deserialize)]
pub struct FooBarJson { ... }

let model = ...;
let hoge: FooBarJson = model.extension("FooBar").unwrap();

which can be implemented like the following:

use gltf_derive::Validate;
use serde::{de::DeserializeOwned, Deserialize, Serialize};
use serde_json::Value;
use std::collections::HashMap;

#[derive(Clone, Debug, Default, Deserialize, Serialize, Validate)]
pub struct Root {
    #[serde(default, flatten)]
    map: HashMap<String, Value>,
}

#[derive(Debug)]
pub enum ExtensionError {
    NotFound,
    JsonError(serde_json::Error),
}

impl From<serde_json::Error> for ExtensionError {
    fn from(e: serde_json::Error) -> Self {
        Self::JsonError(e)
    }
}

impl Root {
    pub fn extension<T>(&self, name: &str) -> Result<T, ExtensionError>
    where
        T: DeserializeOwned,
    {
        serde_json::from_value(self.map.get(name).ok_or(ExtensionError::NotFound)?.clone())
            .map_err(|e| e.into())
    }
}

@alteous
Copy link
Member Author

alteous commented Sep 9, 2019

This is the current approach, minus the extension function. The downside is extensions are deserialized no matter if you use them or not, so it's not zero cost.

@3c1u
Copy link

3c1u commented Sep 14, 2019

If possible, taking extensions as a type parameter will be zero cost.

#[derive(...)]
pub struct VrmModelExtensions {
    ...
}

let model: Gltf<VrmModelExtensions> = Gltf::open("model.vrm")?;

The downside is, that these custom Gltfs are not compatible with each others (i.e. not easy to cast from one to another).

@alteous
Copy link
Member Author

alteous commented Sep 21, 2019

@3c1u: we have experimented with this approach before. It was a user experience disaster! 😄

#11 (comment)

@alteous
Copy link
Member Author

alteous commented Sep 21, 2019

There is a likely issue with requiring T: DeserializeOwned: it requires the Deserialize implementation to be the same version the crate expects.

zellski added a commit to facebookincubator/glTFVariantMeld that referenced this issue Oct 5, 2019
We're going to need to use a forked version of these crates for now.

For one thing, we need to implement FB_material_variants, one way or
another. It could be added to the gltf-rs/gltf fork, as one of the
hard-coded extensions, but that seems irritating. It's really only a
concern for glTFVariantMeld.

The other is to implement some simple way to implement user extension
support; something like gltf-rs/gltf#231.
@adrien-ben
Copy link
Contributor

adrien-ben commented Oct 18, 2019

Hi, in the meantime, do you accept PR adding support for specific extensions ? Here is one to support KHR_materials_unlit #263

Edit: For those interrested it's been merged to master

@jczh98
Copy link

jczh98 commented Apr 28, 2022

Have there any updates for custom extensons?

@expenses
Copy link
Contributor

expenses commented Jun 7, 2022

I think I'm going to look into this specifically for VRM support. For that, all the extension info is in the root extensions property so I'll try and find a way to implement extending that.

@bkolligs
Copy link

bkolligs commented Aug 3, 2023

Any updates for custom extensions?

@alteous
Copy link
Member Author

alteous commented Aug 4, 2023

No major updates. The proposed solution hasn't changed, just not implemented. The type parameter as suggested previously (#231 (comment)) will not be considered.

@floppyhammer
Copy link

Is there any workaround that can currently be used to access raw extension data?

@aaronfranke
Copy link

What is the policy on adding built-in support for extensions? Just KHR ones or what about other extensions?

I work on Godot Engine, which has support for OMI physics, and in the future KHR physics once that is finalized. Would gltf-rs be interested in having support for importing physics information if I wrote the code and submitted it as a PR?

@alteous
Copy link
Member Author

alteous commented May 31, 2024

@aaronfranke if any extension is stable, KHR or otherwise, then built-in support is welcomed.

I am curious about the motivation. Is this crate used by Godot in some way or would it be to promote the extension more generally?

The project is experiencing some growing pains at the moment. I'm working on a major overhaul to simplify addition of extensions, so you might want to wait for that first. Nonetheless, any extensions in the current code will be ported to the new code.

Edit: fixed link.

@aaronfranke
Copy link

aaronfranke commented Jun 1, 2024

@alteous Godot has its own glTF import and export pipeline, it does not use this crate. The purpose would be to provide a path to easily support these extensions in Rust projects such as Bevy, allowing for the interchange of more advanced content in the glTF ecosystem with Rust projects as another endpoint.

As for stability, that depends on the specific extension. Some things are very stable, some things mostly have a stable base but may have more added to them, and a few things are not considered stable yet. Anything that is not considered fully stable can be left as a draft pull request, as long as it can be accepted in the future once the extension becomes stable. Before stability, I would still like the ability to create proof-of-concept implementations of an extension, and want to do it in a way there this can turn into a final merged feature when the extension has matured.

Anyway, I will wait until the major overhaul is done before working on this. Feel free to ping me if you remember, otherwise I'll try to check back later. How can I check your progress, should I just wait until that branch you linked has been deleted? Will you be opening a draft PR that I can subscribe to so that I will get notifications?

@alteous
Copy link
Member Author

alteous commented Jun 3, 2024

See #422 for the rework. 😄

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

Successfully merging a pull request may close this issue.

9 participants