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

Rework for version 2.0 #422

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alteous
Copy link
Member

@alteous alteous commented Jun 3, 2024

No description provided.

Fix tests

Simplify json::animation module

Simplify json::buffer module

Simpify json::camera module

Simpify json::extensions::scene module

Simpify json::material module

Simplify json::mesh module

Simplify json::mesh module

Simplify json::texture module

Remove json::validation::Checked enum

Replace unnecessary macro

Merge gltf-json crate into gltf crate

Simplify json::camera module some more

Add stub wrapper code

Trial generating wrapper code

Replace Void type with IgnoredAny

Remove unused Primitive type

Add wrap_indexed extension

Remove unnecessary const

Add custom (de)serialization macros

Remove accessor extensions

Remove animation extensions

Remove asset extensions

Remove buffer extensions

Remove image extensions

Remove skin extensions

Insert serde attributes automatically for Option

Insert serde attributes automatically for Vec

Test and fix auto camel case fields

Add tests for gltf(extension) attribute

Fix doc test builds

Temporarily remove extras feature

Refactor asset

Temporarily remove names feature

Refactor buffer

Insert serde attributes automatically for bool

Refactor accessor

Refactor animation

Refactor camera

Refactor image

Add default field attribute and derive macro

Refactor material

Refactor scene and root

Refactor texture

Refactor material and mesh

Refactor skin

Remove re-exports of serde_json functions

Refactor remaining modules

The giant hoist

Move Wrap trait into its own module

Refactor lib.rs

Implement Index and IndexMut for Root

Test camera projection

Rename accessor::Type as AttributeType

Remove leftovers

Add fallback for unrecognized extensions

Refactor import
@kpreid
Copy link
Contributor

kpreid commented Jun 3, 2024

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
Copy link
Member Author

alteous commented Jun 4, 2024

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

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.

* Replace `buffer::Stride` with `usize`
* Avoid serializing `buffer::View::offset`

Additional changes:

* Rename validate_hook as validate
* Allow `#[gltf(validate = "...")]` to be applied to fields
* Remove remaining gltf-json code
* Document `gltf_derive::Default` macro
* Document `gltf_derive::Validate` hook
@kpreid
Copy link
Contributor

kpreid commented Jun 4, 2024

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
Copy link
Member Author

alteous commented Jun 26, 2024

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
Copy link
Contributor

kpreid commented Jun 26, 2024

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
Copy link
Member Author

alteous commented Jun 27, 2024

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
Copy link
Member Author

alteous commented Jul 3, 2024

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
Copy link
Contributor

kpreid commented Sep 23, 2024

I notice that in the current state of the branch (9b8bc87), 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
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

3 participants