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

gltfpack: Enhance .obj parsing by preserving more information #596

Merged
merged 7 commits into from
Aug 23, 2023

Conversation

zeux
Copy link
Owner

@zeux zeux commented Aug 21, 2023

When parsing .obj, gltfpack now:

  • Preserves material names
  • Preserves object names by parsing individual objects separately and assigning them to separate nodes
  • Preserves Kd/d material parameters
  • Fixes texture paths if they contain backslashes in the source .mtl
  • Assigns the default scene in the output .glb file

Before merging:

  • Review and test more thoroughly
  • Measure memory/performance overhead of object split/merge on a few large scenes

Fixes #594
Fixes #595

Also when the material is using different texture paths for diffuse and
alpha, warn about the mismatch - we can't represent a separate alpha
mask via standard glTF materials.
@zeux
Copy link
Owner Author

zeux commented Aug 22, 2023

Looks like some .obj exporters use objects and some use groups to split the data :-/ this doesn't affect correctness of conversion but does mean this PR won't handle some input scenes depending on the exporter that was used.

Objects are more popular in my test data set but groups still occur fairly frequently. For now it's probably fine, as groups are more equivalent to meshes but gltfpack doesn't keep mesh names anyway...

zeux added 2 commits August 21, 2023 18:11
These will be preserved in the output glTF file, and using -km option
will disable merging named materials.

Also define _CRT_NONSTDC_NO_WARNINGS to avoid MSVC warnings for strdup.
We now assign a node to each individual object in .obj file; this allows
us to preserve object names. When -kn is not specified, gltfpack will
merge the meshes with the same materials anyway so the result should not
change, but with -kn the nodes will be preserved in the output file.
@zeux zeux force-pushed the gltf-objext branch 2 times, most recently from 7f33cc7 to 39e1476 Compare August 22, 2023 01:16
zeux added 2 commits August 21, 2023 18:17
This change splits nodes and materials parsing and keeps mesh group
parsing loop so that the code looks more similar to what we used to have
when we just had one group. That will also make it easier to ensure the
relationships between objects and nodes stay 1-1.
@kzhsw
Copy link

kzhsw commented Aug 22, 2023

Also consider the roughness factor here, the textures here, and transparent-based doubleSided here

@zeux
Copy link
Owner Author

zeux commented Aug 22, 2023

The textures in .obj are typically using the spec-gloss model. They could be emitted via KHR_materials_pbrSpecularGlossiness, however this extension is deprecated in favor of KHR_materials_specular I think - either way this requires a fair amount of special cased work which I don't think makes sense to do absent specific important use cases. Similarly I don't want to just tackle the roughness factor without considering the rest, so I'd rather keep the model simple (just a color + texture) for now. I would not be surprised if obj2gltf converts textures from spec+gloss to metal+roughness inferring the individual pixel values - this is very much out of scope of gltfpack.

Inferring double-sidedness based on transparency is easy but kinda specific... I'm not exactly sure what the motivation there is.

@kzhsw
Copy link

kzhsw commented Aug 22, 2023

The emissiveTexture and normalTexture here does not seems to be part of the spec-gloss model, could this be added?

@zeux
Copy link
Owner Author

zeux commented Aug 22, 2023

Yeah these should work without issues I believe as they fit as is within any material model even without the use of extensions, I'll add them.

@zeux
Copy link
Owner Author

zeux commented Aug 22, 2023

Hmm, actually, obj2gltf seems to assume map_bump in .mtl files refers to normal maps, but Sponza example on https://casual-effects.com/data/ at least assumes map_bump refers to actual bump maps (containing height data). Unsure if obj2gltf is correct here, or what the spec says if any.

@zeux
Copy link
Owner Author

zeux commented Aug 22, 2023

Oh boy :-/ https://paulbourke.net/dataformats/mtl/ says:

During rendering, the map_Kd value is multiplied by the Kd value.

... which is consistent with glTF spec's treatment of baseColorFactor & baseColorTexture. But of course, some files I am looking at have Kd at 0 and map_Kd set to a texture, so the effect of this change is that they render as black. obj2gltf ignores Kd when map_Kd is present which looks like the correct course of action in absence of an official spec.

zeux added 2 commits August 22, 2023 07:12
Some .mtl files contain backslash-delimited paths; backslash is not
valid as a delimiter in URIs and paths like this can't be opened on
non-Windows platforms when textures are embedded or compressed.
It's not fully clear what the expected behavior is when both Kd and
map_Kd are specified; some online documentation claims that they are
multiplied, but some .obj files in the wild have map_Kd and Kd=0, so to
stay safe we now only apply Kd/d when there's no map with the same name.
@zeux
Copy link
Owner Author

zeux commented Aug 23, 2023

Given the amount of irregularities in .mtl I think this is a reasonable stopping point. Future additions will need to be evaluated on a case-by-case basis. map_bump has the aforementioned ambiguity that makes me think that absent a thorough analysis of whether one variant is common place it's not a good idea, and I wasn't able to find reasonable examples for emissive so for now both are not implemented.

@zeux zeux merged commit 9c6759b into master Aug 23, 2023
@zeux zeux deleted the gltf-objext branch August 23, 2023 01:09
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.

gltfpack: keep obj hierarchy and name gltfpack: obj material color
2 participants