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: Merge occlusion, roughness and metalness textures #23229

Merged
merged 4 commits into from
Jan 20, 2022

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jan 14, 2022

Related issue: #14940 #16325

Description

The code in this PR only checks if texture has already been merged (which solves the actual issue in #14940).
Should we add additional parameters as suggested in #14940 (comment)?

/cc @fernandojsg @donmccurdy @elalish

@mrdoob mrdoob added this to the r137 milestone Jan 14, 2022
@mrdoob mrdoob requested a review from donmccurdy January 14, 2022 17:16
@mrdoob
Copy link
Owner Author

mrdoob commented Jan 20, 2022

This will need some tweaks probably but it'll easier to understand when people start using it. Merging.

@mrdoob mrdoob merged commit fd3983a into dev Jan 20, 2022
@mrdoob mrdoob deleted the gltfexporter branch January 20, 2022 19:00
@donmccurdy
Copy link
Collaborator

This looks good to me!

I'd also be fine with omitting .aoMap from the merging. While glTF has other material textures that can be merged (e.g. thickness texture only uses r), roughness/metallic are the only textures that must be merged, excluding the spec/gloss workflow. I don't think GLTFExporter should be automatically merging anything except O/R/M.

We could probably also change this line to just use PNG, if we're moving away from RGBFormat:

const mimeType = format === RGBAFormat ? 'image/png' : 'image/jpeg';

^does that seem okay @Mugen87 ?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 27, 2022

Yes, that makes sense!

@donmccurdy
Copy link
Collaborator

Opened #23385.

@repalash
Copy link
Contributor

Hello, in the buildOrmTexture function, a Texture.image is returned when the textures are same on line 698, and a Texture is returned at 759. This gives an error when they are equal because processTexture expects a Texture.

Also, in the case where occlusion is null and roughness == metalness, what is the need to recreate the texture then?

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 11, 2022

Hello, in the buildOrmTexture function, a Texture.image is returned when the textures are same on line 698, and a Texture is returned at 759. This gives an error when they are equal because processTexture expects a Texture.

#23463 fixed that.

Also, in the case where occlusion is null and roughness == metalness, what is the need to recreate the texture then?

Yes, that logic needs to be improved.

@repalash
Copy link
Contributor

Thanks for your reply.
@mrdoob In the case where the same aoMap is applied to multiple objects, will it be serialized separately for each object or is this handled somewhere?

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 11, 2022

@mrdoob In the case where the same aoMap is applied to multiple objects, will it be serialized separately for each object or is this handled somewhere?

Oh, I was not aware that could happen. Does that also happen with metalnessMap and/or roughnessMap?

@repalash
Copy link
Contributor

Does that also happen with metalnessMap and/or roughnessMap?

Generally not. aoMaps use uv2, so it's possible to pack occlusion for multiple objects or even the complete scene in a single texture.

@mrdoob
Copy link
Owner Author

mrdoob commented Mar 1, 2022

@repalash Fixed #23616

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.

4 participants