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

GLTFExporter: Support metallicRoughnessTexture #13415

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

takahirox
Copy link
Collaborator

This PR adds metallicRoughnessTexture support to GLTFExporter.

In Three.js metalnessMap and roughnessMap can be different textures but can't be in glTF 2.0. So exporting only if they are the same texture.

@takahirox takahirox mentioned this pull request Feb 23, 2018
87 tasks
@mrdoob mrdoob added this to the r91 milestone Feb 23, 2018
@mrdoob mrdoob merged commit b1227ff into mrdoob:dev Feb 23, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2018

Thanks!

@donmccurdy
Copy link
Collaborator

Oh just realized, ideally if occlusionMap is the same texture we shouldn’t process it again.

@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2018

Ah yeah... aoMap in three.js terms.

@takahirox
Copy link
Collaborator Author

Ah, you meant that's from OcclusionRoughnessMetallic compatibility. I'll fix.

@takahirox takahirox deleted the GLTFExporterMetallicRoughnessTexture branch February 24, 2018 13:45
@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2018

We have to revisit this...

If you only use a roughnessMap texture the exporter ignores it because roughnessMap !== metalnessMap.

Also... Maybe the exporter could build the composite texture for the user?

@takahirox
Copy link
Collaborator Author

takahirox commented Feb 27, 2018

In glTF metallic-roughness texture, B channel is for metalness and G channel is for roughness.

If you only use a roughnessMap texture and the exporter exports it as metallic-roughness texture, its B channel will be unexpectedly (for exporter user) used. So I let the exporter ignore the texture in that case.

Probably we could composite and, I need to review the metallic-roughness calculation but, in the case even one of them is used perhaps we could make up by filling 0 or 1 in a certain channel?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Feb 27, 2018

Also... Maybe the exporter could build the composite texture for the user?

If we want to get into image manipulation, we could also see about converting alphaMap to the alpha channel of baseColorTexture... maybe worth putting that code into a helper method:

var blob = ImageUtils.composite({
  r: 1,
  g: roughnessTexture,
  b: metalnessTexture
});

@takahirox
Copy link
Collaborator Author

takahirox commented Feb 27, 2018

I let the exporter ignore the textures in those cases because I wanted to quickly implement. I think compositing is good for users.

And, as Don mentioned, doing that in helper method is nicer than doing in the exporter. It can be reusable from other modules and users (could be used for optimization?).

@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2018

👍👍👍

@takahirox
Copy link
Collaborator Author

takahirox commented Feb 27, 2018

we could also see about converting alphaMap to the alpha channel of baseColorTexture

That could be nice, map A channel * alphaMap => baseColorTexture A channel. User won't lose alphaMap info.

@takahirox
Copy link
Collaborator Author

BTW tho I said it's good to users, do you think there're many models using separated metalness/roughnessMap? I suppose (O)RM (occlusion/roughness/metal) is packed into one texture (called ORM texture) in many cases.

@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2018

BTW tho I said it's good to users, do you think there're many models using separated metalness/roughnessMap?

I was trying to build my own. I was trying to add this texture (albeit is inverted) as roughnessMap to the world test. But I can't do it because the exporter expects me to add a metalnessMap too.

@takahirox
Copy link
Collaborator Author

I see. Yeah seems better to compose/make up not to lose info as much as possible.

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.

3 participants