CesiumGS / obj2gltf

Convert OBJ assets to glTF
Apache License 2.0
1.71k stars 307 forks source link

fix texture paths and parse texture map options #109

Closed timknip closed 7 years ago

timknip commented 7 years ago

When the mtl has statements like map_Bump -bm 0.2 ./foo.jpg then the options end up in the texture path. eg: ./-bm 0.2/foo.jpg.

This commit fixes the path and parses the options.

Note: loadMaterialTexture is probably a bad location to do this, but i needed a solution fast.

Tests / eslint all pass.

lilleyse commented 7 years ago

Thanks @timknip, I'll check this out soon.

Have you sent in a CLA https://github.com/AnalyticalGraphicsInc/obj2gltf#contributions?

timknip commented 7 years ago

Hi @lilleyse, I just signed and send this agreement to cla@agi.com. That enough?

lilleyse commented 7 years ago

Yup that's enough.

pjcozzi commented 7 years ago

Thanks again for the pull request @timknip, we received your CLA!

timknip commented 7 years ago

@pjcozzi @lilleyse Best see this PR as a head's up. Its bit hackish code. I'm not into glTF enough to actually do something with these parsed texture map options.

For example the -s (texture scale) and -t (texture translation) options: how would that be done in glTF? sampler could not do it, could it? Perhaps modifying the texture itself? The -bm (bump factor) might be easier. Perhaps that one can be done in material? Or alternatively simply modify the texture again (just multiply the pixel values with the bump factor). Modifying textures of course means that when multiple materials use the same texture with different options, the texture needs to be copied (growing resulting file size). Another interesting thing in MTL is the illum param which afaik is not handled at the moment.

0. Color on and Ambient off
1. Color on and Ambient on
2. Highlight on
3. Reflection on and Ray trace on
4. Transparency: Glass on, Reflection: Ray trace on
5. Reflection: Fresnel on and Ray trace on
6. Transparency: Refraction on, Reflection: Fresnel off and Ray trace on
7. Transparency: Refraction on, Reflection: Fresnel on and Ray trace on
8. Reflection on and Ray trace off
9. Transparency: Glass on, Reflection: Ray trace off
10. Casts shadows onto invisible surfaces

Also here: no clue how to map this to glTF.

If you have some ideas on this, i would love to hear. Sorry for the long post ;-)

lilleyse commented 7 years ago

For now we should just ignore the texture options since they aren't super common and I also can't think of how to map most of them to glTF (except maybe the -clamp option). illum is another case where it's best to just ignore it.

lilleyse commented 7 years ago

Going along with that, my main suggestion for the code is to ignore those values and get the correct texture path (with options filtered out) during the parsing stage.

Each map parse has a line like:

texturePath = path.resolve(mtlDirectory, line.substring(7).trim());

that could become something like:

texturePath = getTexturePath(path.resolve(mtlDirectory, line.substring(7).trim()));

Is the path always listed after the options? If so the regex could be simpler too.

Sorry that these suggestions mean a lot of the code in the PR may be removed, but I think simplicity is best in this case.

timknip commented 7 years ago

@lilleyse Yes, path is always last. Latest commit simply strips the options from the texture name. Only downside is that the code assumes textures don't have spaces in its name.

The problem is that many options have optional params. eg: -o u [v [w]]. So creating a regexp to safely parse the options is difficult.

lilleyse commented 7 years ago

Thanks! Looks good.