gltf-rs / gltf

A crate for loading glTF 2.0
Apache License 2.0
536 stars 124 forks source link

Allow read-only view of Document's JSON root. #250

Closed zellski closed 5 years ago

zellski commented 5 years ago

We're writing a glTF-manipulating tool, where the core operation involves deriving new asset from existing one(s) in various ways. As things stand now, it's awkward to write code that uses the convenient, higher-level gltf:: methods, while also having read access to the underlying gltf::json:: structures, at least without needless cloning.

Ideally I'd love to have read-only access to the underlying JSON for every glTF object, e.g.

    gltf::Material::json(&self) -> &gltf::json::Material;

but failing that, access at the Document level would suffice.

I suspect you have deliberate architectural reasons why you keep the underlying JSON entirely encapsulated, and most of the time I think that's exactly right. The exception I've found, as mentioned above, is when we're building a new glTF file, by building up and extending a gltf::json::Root. This is obviously done with gltf::json:: structs, and at that point the ability to clone-and-tweak JSON structs in the original asset is very valuable.

Thoughts?

alteous commented 5 years ago

The ability to edit glTF has been planned for a while. The current method of editing the internal JSON data structures directly is a stop-gap measure for some other more convenient way. See #234, #187, and #193 for some history. I'm reluctant to merge because (I think?) we used to have these all over but they were misused.

zellski commented 5 years ago

@alteous That makes sense. I like the direction of #234, although it does look like an ambitious undertaking and we're on a pretty tight schedule.

You don't explicitly mention glTF editing in that task, but perhaps once you have a data-driven generator, it'll be comparatively easy to generate mutators alongside?

(There's a bunch of rabbit holes there, though, as I'm sure you've already noted. One of them, which I ran into in another project, is how glTF objects reference one another. With accessors, things work fine with glTF's explicit object arrays that cross-reference one another with integer indexes. It's easy to export those as helpful object references. But once you're in the land of generating new glTF with objects that reference another in non-trivial ways... I found that I really wanted to keep those as actual object references, and only convert them to indexes during a final export step. Your mileage may vary, though!)

I can do without this PR, but unless I'm missing something, in practice it means a copious amount of cloning. I found it impractical to call the self-consuming into_json() for read-only operations, so the only recourse is either to do a full clone on each operation (and call into_json on that), or else to work only with Root... but then I'm basically not even using the gltf crate, just gltf-json.

Can you think of any stopgap measure for this quandary? Am I describing it in a way that makes sense? I'm very new to Rust, so it's entirely possible there's some pattern I'm not thinking of that would make this palatable.

The other option is for me to maintain a fork, but that's kind of a chore for an actively developed project.

I'm still working in a private repo, but we should be moving to an official one shortly, and then I'll be able to show you all the painful ways we use your crate. :-)

zellski commented 5 years ago

I'll withdraw this PR, given your comments above. We've switched to using exclusively gltf::json:: structs for now, which works well enough. Thanks!