Page MenuHome

Fix T97757: Some MTL import correctness issues in the new OBJ importer
ClosedPublic

Authored by Aras Pranckevicius (aras_p) on May 2 2022, 8:25 PM.

Details

Summary

Fix several correctness issues where the new OBJ/MTL importer was not producing the same results as the old one, mostly because the code for some reason had slightly different logic. Fixes T97757:

  • When .obj file tries to use a material that does not exist, the code was continuing to use the previous material, instead of creating new default one, as the previous importer did.
  • Previous importer was always searching/parsing "foo.mtl" for a "foo.obj" file, even if the file itself does not contain "mtllib foo.mtl" statement. One file from T97757 repros happens to depend on that, so resurrect that behavior.
  • When IOR (Ni) or Alpha (d) are not specified in .mtl file, do not wrongly set -1 values to the blender material.
  • When base (Kd) or emissive (Ke) colors are not specified in the .mtl file, do not set them on the blender material.
  • Roughness and metallic values used by viewport shading were not set onto blender material.
  • The logic for when metallic was set to zero was incorrect; it should be set to zero when "not using reflection", not when "mtl file does not contain metallic".
  • Do not produce a warning when illum value is not spelled out in .mtl file, treat as default (1).
  • Parse illum as a float just like python importer does, as to not reintroduce part of T60135.

Now the repro file from T97757 almost matches between the old & new importer:

The difference (selected objects) is where the old importer was producing arguably wrong results -- the vertex normals for the cornell box itself are never set in the .obj file, but the file contains some "completely unused" vertices along with their normals at the end of the file. The old importer was wrongly fetching normals for the cornell box vertices from that; basically the normals are nonsensical. The new importer behaves more correctly here.

Diff Detail

Repository
rB Blender

Event Timeline

Aras Pranckevicius (aras_p) requested review of this revision.May 2 2022, 8:25 PM
Aras Pranckevicius (aras_p) created this revision.
Jesse Yurkovich (deadpin) added inline comments.
source/blender/io/wavefront_obj/importer/obj_import_file_reader.cc
305

Yeah, it's unfortunate. I found 3 other files last night that depended on that same quirk of the old importer (confirmed fixed with this patch now). However, given that the spec says that material libs should be searched in order, should this be appended after the regular parsing so this truly becomes just a last-resort fallback?

source/blender/io/wavefront_obj/importer/obj_import_mtl.cc
200

For consistency, should the code that parses the "illum" value line default to 1 as well if the value is not present? Currently it defaults to 2: parse_int(line, 2, material->illum);

Minor tweaks addressing inline comments by deadpin.

Aras Pranckevicius (aras_p) marked an inline comment as done.May 3 2022, 12:28 PM
Aras Pranckevicius (aras_p) added inline comments.
source/blender/io/wavefront_obj/importer/obj_import_mtl.cc
200

Good catch! I noticed that the python importer was parsing "illum" as a float (apparently some .mtl files have that), so I did that too.

Aras Pranckevicius (aras_p) marked an inline comment as done.May 3 2022, 12:29 PM
Aras Pranckevicius (aras_p) edited the summary of this revision. (Show Details)

Thanks for the fix.

This revision is now accepted and ready to land.May 3 2022, 1:15 PM