Export builders and example

[anderejd]

This PR

This is an initial attempt at improving export ergonomics and correctness. It's intended to help with the most basic export needs, but should be easy to extended later.

Future work/PRs

• More builders and component types to handle more advanced scenes.
• Adding another level of builders on top of these builders could be interesting to allow all of buffer data, buffers, buffer views and accessors to be built by the same builder.

Related to

• Export (GltfBuilder) #187

[anderejd]

Manual benchmarks looks promising, exporting a mesh of 10 000 000 indexed triangles to gltf with external binary buffer on disk takes about half a second on my machine. It's way better than I was hoping for at this stage 🙂

[anderejd]

Rebased and squashed the corrections made since this PR was opened. I think this is ready for review now.

[anderejd]

Sorry in advance for the spam.

Should this and everything related to export be put behind a cargo feature flag or perhaps moved to and published in a new crate? I imagine that only a tiny part of users will be interested in export and moving forward with my project I will be wanting to flesh out the builders with more code. I think a feature flag would be a nice option in case the export helper code would need to access to internals later.

Regarding feature flags, would it make sense to remove the current feature flags and make that functionality standard? From what I have seen the current feature flags enable a few optional properties and don't really change much for the user code? At least the "extras" and "names" seems like they would make sense to remove and have that code enabled in all builds to avoid all the #[cfg(feature = attributes.

[alteous]

Is it possible to not have to call with_options like PrimitiveBuilder?

[anderejd]

Yes, the easiest way would be to drop compile time checks for required vs optional parameters and return a result from .build. Please see my reply #193 (comment) for some follow-up questions.

[alteous]

Wow, thank you very much! This is a great start.

I have a lot of catching up to do. Let me start by answering your questions:

Should this and everything related to export be put behind a cargo feature flag or perhaps moved to and published in a new crate?

Please, not a new crate. :) I'd much prefer a cargo feature flag as they are far easier to maintain than separate crates. Believe me: we've tried this before!

Regarding feature flags, would it make sense to remove the current feature flags and make that functionality standard?

Perhaps. On Gitter and IRC people seem to prefer lean libraries with opt-in features rather than fat libraries with opt-out features. The actual reason why they are opt-out currently is because they are relatively expensive to capture due to being owned and that they are not strictly necessary for an application to import.

Regarding the design, I have two concerns:

1. I don't like the with_options function; please revise this.
2. Is it possible to export interleaved vertex data in a 'safe' way? I envisioned a #[derive(Vertex)]-like thingamajig that would generate the accessor field values automatically.

[anderejd]

Thanks for the feedback!

Please, not a new crate. :) I'd much prefer a cargo feature flag as they are far easier to maintain than separate crates. Believe me: we've tried this before!

Sure thing, feature flag it is! I'll hide all the new code behind a new cargo feature and I'll also leave the current cargo feature flags as is.

I don't like the with_options function; please revise this.

Yeah... I'm not entirely happy with that one either. The primary purpose of the with_options thingy is to force named parameters both for required and optional fields, while avoiding copy pasting : default::Default() for every optional field in the user code. I think named parameters/fields are important when there are many parameters of the same type or just similar numeric types.

I considered marker types like this: https://dev.to/mindflavor/rust-builder-pattern-with-types-3chf but that seems to me to add more complexity than it's worth(?).

Another simple option would be to move the required parameters to a struct called something like SomethingBuilderRequiredParams and take that as the only parameter in a ::new method on the actual builder?

EDIT: The .with_options is the best looking variant I've been able to figure out yet. Please advise how you would like to change it. Is it the naming that's annoying to you? How about simply .options?

Is it possible to export interleaved vertex data in a 'safe' way? I envisioned a #[derive(Vertex)]-like thingamajig that would generate the accessor field values automatically.

I haven't put much thought into interleaved formats yet beyond what's provided in the doc-comments. At this stage I'm leaning towards the feeling that interleaved formats will require a completely different code path with its own dedicated builder and Read implementation to avoid unnecessary growth of code complexity.

[anderejd]

Closing due to inactivity.

I'm moving this code back inside my project and will develop it there until ready. I want support for Iterator instead of slices to avoid large extra allocations on export as well as support for interleaved formats. A higher level export builder would also be nice. This is likely going to take quite some time until I'm satisfied with the overall shape of things.

I'll open a new PR, when it's done.

[anderejd]

Should we reopen this?

[alteous]

Yes, I have a weekend free to work on this. :)

[alteous]

The interesting case is being able to prepare buffer data for export and I'm not sure how to approach this.

Generating builders for the json data structures is straightforward. I'm experimenting with the derive_builder crate for this and that seems to work well.

[anderejd]

I'm going to be AFK for most of this weekend so I'll try to make a proper braindump in this reply.

The interesting case is being able to prepare buffer data for export and I'm not sure how to approach this.

The way I went about this for this PR was to work towards a specific use case: exporting high resolution triangle meshes. The example in this PR only exports a single triangle but I can provide a meshlite based high-res export example if you think that would be useful, it will look almost identical to the existing example but with some meshlite glue code added.

To push this PR forward I would add more examples of increasing complexity to drive the API design, perhaps using three-rs as an export data source for some of the more advanced examples.

We did start some initial code review a couple of months ago in this PR, I'm not sure how you want to resolve that, feel free to modify this PR as you see fit. I'm pretty pleased with how the API looks and works for the current export example, but I suspect that it'll need to evolve when adding more complex export examples. If you feel unsure about evolving the export API inside gltf-rs due to API churn, we could just as well do this in a new crate and let that crate stay in 0.x mode until it's ready.

To summarize, I would recommend adding more examples of increasing complexity and build out the export API based on the needs of the examples.

Hope that helps, I'll have more time next week. Have a nice (and productive!) weekend :)

[aloucks]

This looks promising! Are there plans to merge this in the near term?

One issue is that the Primitive attributes are stored in a HashMap with RandomState. The serialized output won't be deterministic given the same input. I think it would be better to change this to use BTreeMap or use a deterministic hash state (e.g. FnvBuildHasher).

[anderejd]

One issue is that the Primitive attributes are stored in a HashMap with RandomState. The serialized output won't be deterministic given the same input. I think it would be better to change this to use BTreeMap or use a deterministic hash state (e.g. FnvBuildHasher).

Good point! Let's open an issue to change that in Primitives as well.

This looks promising! Are there plans to merge this in the near term?

I would like to push this PR to an acceptable state for merging but it seems like I'm waiting for more feedback while @alteous could be waiting for me to provide more code 😄

I want to push this forward, how would you like to proceed? Review and change this PR or close and move it to a new crate? Please don't hesitate to close it if you think this is going in the completely wrong direction to be merged in this crate.

[bwasty]

If @alteous doesn't find the time, I'll try to have a proper look at this soonish.

[alteous]

@anderejd : sorry but I want to move the crate in a different direction. See #234 for the details. Thank you for your work nonetheless.

[anderejd]

No problem :)