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

Move to option based API for materials #60

Merged
merged 2 commits into from
Apr 29, 2023

Conversation

luca-della-vedova
Copy link
Contributor

⚠️ Breaking Change ⚠️

Proposed fix for #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

Signed-off-by: Luca Della Vedova <[email protected]>
@Twinklebear
Copy link
Owner

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

@Twinklebear
Copy link
Owner

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.

@Twinklebear Twinklebear merged commit 226cd79 into Twinklebear:master Apr 29, 2023
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.

2 participants