Rework for version 2.0

[kpreid]

As someone interested in the 2.0 plan, and with an application that exports to glTF, I tried out this branch, and here are my observations:

• gltf::buffer::View is serialized with the byteOffset field even if it's zero — this seems inconsistent with other serialization where the default value is always omitted.
• It'd be nice if gltf::buffer::Stride implemented TryFrom<usize>.
• Have you considered making the serialization structs #[non_exhaustive]? This would make it not a semver-breaking change to add new extension fields, at the cost of requiring applications that create them to use ..Default::default().

[alteous]

@kpreid thanks for trying it out and providing feedback so quick! :smile:

byteOffset not being serialised is a bug, well spotted.

TryFrom<usize> should be a quick fix. I'm not super happy with these new types from an ergonomic point of view. The USize64 type is particularly annoying to work with. I'm considering converting these to a type alias instead and perhaps having the type configurable. The name of the type alias could be recognised as a special case by the derive macro if required.

Regarding #[non_exhaustive], it's not a bad idea, the trouble is some data structures are unsuitable for having a Default implementation. Index values are particularly problematic but there are other examples. I thought about having a 'base' constructor for these cases like this:

#[derive(Debug)]
#[non_exhaustive]
pub struct Example {
    pub index: usize,
    pub required: i32,
    pub optional: Option<i32>,
}

impl Example {
    pub fn base(index: usize, required: i32) -> Self {
        Self {
            index,
            required,
            optional: None,
        }
    }
}

#[test]
fn exhaustive() {
    let example = lib::Example  {
        index: 123,
        optional: Some(456),
        required: 789,
    };
    println!("{example:#?}");
}

#[test]
fn non_exhaustive() {
    let example = lib::Example  {
        optional: Some(456),
        ..lib::Example::base(123, 456)
    };
    println!("{example:#?}");
}

I'm not sure about the ergonomics of this approach though.

[kpreid]

converting these to a type alias instead and perhaps having the type configurable.

Changing a type alias would be a non-additive feature (regardless of the direction it goes; both widening and narrowing can break code), which becomes a hazard for any use of gltf from a library crate (which may exist in the same dependency graph as other libraries that prefer the opposite configuration)..

Regarding #[non_exhaustive], …

That all makes sense. I just wanted to ensure you'd considered the option when designing 2.0. I personally wouldn't mind the “base constructor” design, but my preferences tend to rigor over convenience.

[alteous]

I've given the base constructor a go and it's really not ideal. Here are excerpts of some real code that uses my prototype:

root.push(
    gltf_ext::buffer::View {
        stride: Some(vertex_size),
        target: Some(gltf_ext::buffer::Target::ArrayBuffer),
        ..gltf_ext::buffer::View::new(
            buffer,
            vertex_buffer_view_size,
        )
    },
);

root.push(
    gltf_ext::Accessor {
        buffer_view: Some(vertex_buffer_view),
        count: num_vertices.into(),
        component_type: gltf_ext::accessor::ComponentType::F32,
        extras: Default::default(),
        attribute_type:gltf_ext::accessor::AttributeType::Vec3,
        min: Some(json!(bbox.min())),
        max: Some(json!(bbox.max())),
        ..gltf_ext::Accessor::packed(
            gltf_ext::accessor::AttributeType::Vec3, 
            gltf_ext::accessor::ComponentType::F32, 
            vertex_buffer_view, 
            0u64.into(),
            num_vertices.into(),
        )
    },
);

What I don't like about it is it's unclear what fields are being initialised by the base constructor. It provides limited 'safety' in that there will be fields that need to be set or not set depending on context.

I haven't been able to think of a simple solution to work #[non_exhaustive] into the data structures without it being super annoying in one way or another. I do think it is important we have a way of maintaining semver though. I'm leaning towards feature gating all additional members. We had this before but I removed it because I also thought that was annoying. In hindsight, it does prevent us from breaking semver when we add a new field/extension.

[kpreid]

I do think it is important we have a way of maintaining semver though. I'm leaning towards feature gating all additional members. We had this before but I removed it because I also thought that was annoying. In hindsight, it does prevent us from breaking semver when we add a new field/extension.

Unfortunately, while this strategy satisfies semver, it violates feature additivity. Example: if

• package foo depends on bar and baz
• package bar depends on gltf with features = ["ext_one"]
• package baz depends on gltf with features = ["ext_two"]

then bar sees the fields defined by ext_two, and baz sees the fields defined by ext_one, so both bar and baz will fail to compile due to extra fields (unless they strictly act as if the structs were #[non_exhaustive] anyway).

[alteous]

Ah, you're totally right. In that case, I might have to abandon the one data structure approach and go back to having separate structs for core fields and extensions.

[alteous]

A friend of mine suggested to avoid the semver issue by making a major version release each time a new extension is added. I think it kind of defeats the point of semver in the first place but it would solve this backward compatibility issue.

[kpreid]

I notice that in the current state of the branch (9b8bc87d4fc096594baa17661b17f35f85e475eb), gltf doesn't compile unless features = ["import"] is enabled. It looks like this is because the #[cfg(feature = "import")] were removed, but the relevant dependencies are still optional. I hope gltf 2.0 will still make the importing code and its dependencies (image, etc) optional so that export-only applications can exclude it.

— On the other hand, considering all of the field trouble, maybe an overall better answer is to just make every struct #[non_exhaustive] (not constructible by dependents) and declare that gltf is only for imports, not exports. This solves the feature-additivity problems (since no dependent can be broken by adding a field to a #[non_exhaustive] struct) and exporters can define their own, more single-purpose serialization structs. But exporters then lose the advantage of using structs already carefully written against the glTF specification.

[ShaddyDC]

Somewhat off-topic but related, but it might be worth using cargo-hack with --each-feature in the CI to ensure that all features and their absence compile correctly and without warning.