Semantics for missing fields in mtl files

[luca-della-vedova]

Hi and thanks for the great crate!

I have been using it for some time and noticed that fields in material files are a mix of Option (for now only illumination_model and plain types (all the rest).

I was wondering if they should generally be Options instead? The fact that they are plain data types makes it harder to figure out whether they were effectively parsed in the mtl, thus the library user should use them in their application, or they were not present and just assigned a default value, hence users should fallback to their "default" behavior.

Depending on the types this API has minor, or more serious, repercussions. For String types such as texture names it's not too bad, one can just do

// Current API
if !mat.diffuse_texture.is_empty() {
    [...]
}

// Option based API, generally more idiomatic
if let Some(diffuse_texture) = mat.diffuse_texture {
    [...]
}

However for colors it's a lot more tricky and prone to error, because of the Default implementation here, it's impossible for a user to know whether, for example, the diffuse color was set to black in the mtl file (hence they should want to render it black) or not set at all (in which case they might want to keep the default set by their rendering engine). Now the diffuse color is somewhat of a pathological case since I would be very surprised to find an mtl file without it, but other fields (i.e. shininess, dissolve) are much more likely to be absent and having default values makes it tricky to figure out whether they were in the mtl or not.

I wonder if I missed anything in my understanding of the API, and whether you think it would make sense to move (in a breaking release) to an Option based API so users can easily tell whether a field was present or not? My understanding is that the behavior is designed to mimic tinyobjloader, which is C based and doesn't use pointers so it value initializes all the fields (here). However there is already a discrepancy in the illumination_model in this crate that is modeled as an Option and the illum field that is a plain int in tinyobjloader, hence it can maybe be justified to make the other fields options as well?

What do you think?

[Twinklebear]

Hi @luca-della-vedova , thanks for opening this issue! I think you make a good point, as you mention I had written the original library to match tinyobjloader and with the assumption that MTL materials would always specify the ambient, diffuse, specular and shininess properties . However the MTL file format is kind of underspecified, and some files found in the wild might not specify those properties.

I'd be fine with making a breaking change that turns those material properties into Options as well.

[AmionSky]

As an example Blender does not export diffuse by default when using a material with an image texture.

Going to Options is probably the right approach, but I also wonder if the default for diffuse should be 1.0 as it will not cause issues in case only a diffuse texture was specified.

[Twinklebear]

Closed by #60 . I think having the diffuse always be an option is ok instead of 1.0 as the default, since the app loading the material can see no value was specified and decide what to do with the material.