search

gltf-rs / gltf

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

Add `Root::push()` and `Index::push()` to assist with glTF construction. #404

Closed kpreid closed 11 months ago

kpreid commented 11 months ago

These methods reduce the amount of boilerplate and possible mistakes involved in constructing a glTF asset from scratch.

Most use cases are served by Root::push(); Index::push() is useful for cases like animation samplers where the vector of objects is not in the Root, or in algorithmically complex situations such as pushing both within and outside closures, which would produce borrow conflicts if &mut Root was required for everything.

This requires some expansion of the Get trait to support writing. By keeping the old Get::get(), this is not a breaking change unless some dependent implements Get, but if we choose to make general breaking changes, it would probably make sense to rename the trait, and perhaps even declare it an implementation detail, seal it, and hide its methods. (This PR no longer modifies Get and instead adds AsMut<Vec<T>> implementations.)

This is based on a helper function I wrote and used extensively in my own code, and I've also verified that these methods work well in the same situations.

alteous commented 11 months ago

In spirit, I like it. I must admit I have written similar functions. I'm hesitant on the naming/repurposing of Get though.

Do you think we even need the Get trait with this your change anymore? Perhaps AsRef<[T]>/AsMut<Vec<T>> would serve the same purpose?

kpreid commented 11 months ago

I'm hesitant on the naming/repurposing of Get though.

Indeed, that's the part of this change I'm most unsure about.

Do you think we even need the Get trait with this your change anymore? Perhaps AsRef<[T]>/AsMut<Vec<T>> would serve the same purpose?

That seems like a reasonable approach, and I like it a little better than the two methods read_by_type() / write_by_type() I made up on the spot. I'm generally a little skeptical of making use of very general traits to provide rather application-specific semantics, because there in future cases there might be semantic conflict between what the trait means in general and how it has to be implemented to provide this feature, that even if not really breaking might mean the type can be used generically in a nonsensical way, but I can't think of a reason why this particular case would be risky. And an upside would be that this PR could then be purely additive / non-breaking, since Get could be left exactly as-is.

Would you like me to rewrite this PR in terms of AsRef + AsMut?

alteous commented 11 months ago

Yeah I think we should try AsRef and AsMut.

kpreid commented 11 months ago

Yeah I think we should try AsRef and AsMut.

Done. It turned out that this could be a totally non-breaking change, leaving Get alone, so I did that, but I still have the code for replacing Get, so it can become a separate refactor/API-change PR later.

alteous commented 11 months ago

Thanks! :smile: