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

fix texture paths and parse texture map options #109

Merged
merged 7 commits into from Oct 12, 2017

Conversation

timknip
Copy link

@timknip timknip commented Sep 29, 2017

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.

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

This commit fixes the path and parses the options.
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.
@lilleyse
Copy link
Contributor

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

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

lib/loadMtl.js Outdated
var currPart = null;

mapOptions.reduce(function (p, part) {
if (/-/.test(part)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess that should be a better regexp. It may now catch negative values.

@timknip
Copy link
Author

timknip commented Sep 29, 2017

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

@lilleyse
Copy link
Contributor

Yup that's enough.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 30, 2017

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

@timknip
Copy link
Author

timknip commented Oct 1, 2017

@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 ;-)

This happens when in `*.mtl` a relative texture path does not have a
`./` prefix.
eg: map_bump -bm 0.1 foo.jpg
@lilleyse
Copy link
Contributor

lilleyse commented Oct 9, 2017

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
Copy link
Contributor

lilleyse commented Oct 9, 2017

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
Copy link
Author

timknip commented Oct 10, 2017

@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.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the edits @timknip, just one style comment.

lib/loadMtl.js Outdated
return name.split(/\s+/).pop();
}
return name;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 4 spaces for each indent rather than 2.

@lilleyse
Copy link
Contributor

Thanks! Looks good.

@lilleyse lilleyse merged commit a3c2c38 into CesiumGS:master Oct 12, 2017
lilleyse added a commit that referenced this pull request Nov 1, 2018
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.

None yet

4 participants