search

gltf-rs / gltf

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

follow spec to change more fields to optional #312

Closed wsw0108 closed 2 years ago

wsw0108 commented 3 years ago

Follow the spec https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html

aloucks commented 3 years ago

I have mixed feelings about this. The current behavior will fill the attribute with the default value when it's not present. This PR seems like more of an optimization for serialization. Perhaps an alternate solution would be to skip serialization when the value is equal to the default value?

It's going to be a lot more cumbersome to have to consult the spec to discover the default values with this change. One way to mitigate this would be to provide public constants with the defaults.

e.g.

let double_sided = material.double_sided.unwrap_or(Material::DEFAULT_DOUBLE_SIDED);

But this begs the question: What should something like glfs_json::Material::default() produce in this case? Should the default for double_sided be None or Some(false)?

I think the most well-rounded solution would be to skip serialization when the value equals the spec-default, and only change things to Option<T> if/when there is no default value.

IcanDivideBy0 commented 2 years ago

I agree with @aloucks on this, as a user of this crate, i don't want to spend my time browsing the gltf spec to find what is the default value for each property. The actual behavior seems fine to me, so aside from saving a few bytes from the exported gltf, i don't see much value here. Otherwise, having the default value accessible as a struct constant seems reasonable, it also makes loading your models cumbersome where it doesn't need to.

wsw0108 commented 2 years ago

Thanks for your comments, then I will close it.