search

Twinklebear / tobj

Tiny OBJ Loader in Rust
MIT License
233 stars 47 forks source link

Move to option based API for materials #60

Closed luca-della-vedova closed 1 year ago

luca-della-vedova commented 1 year ago

:warning: Breaking Change :warning:

Proposed fix for https://github.com/Twinklebear/tobj/issues/59

The issue itself describes what this change is trying to address. Material fields are now Options which means users will be able to know if a field was not present, instead of having a default value provided.

I took the liberty of doing a minor refactor to reduce the number of match statements / lines of code, as well as just deriving Default where the a struct's Default implementation was just a Default for all of its fields, namely in Mesh and Material. The former was always possible, while the latter is only possible after this change since some values were initialized to 1.0.

The bulk of the change is in example / test code, I'm not 100% happy with adding the unwrap calls in the test but not sure how to do it otherwise without adding quite some boilerplate. I'm also not very happy with what the parse_float and parse_float3 functions look like, they save a bunch of code but are not very pretty

Twinklebear commented 1 year ago

Thanks @luca-della-vedova ! I'll find some time to review the PR fully this week

Twinklebear commented 1 year ago

I think these changes look good, having to do the try_into in parse_float3 is a bit of a pain, but it looks like it's not possible to collect directly into a fixed size array, so it's ok as it is.