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

OBJ / MTLLoader: Label and convert textures and colors correctly in MTLLoader #23296

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #23283

Description

Convert colors to Linear and label color textures as sRGB when loading MTL materials. Also update the OBJ demo with MTL files to use outputEncoding = sRGBEncoding.

https://raw.githack.com/gkjohnson/three.js/srgb-obj/examples/webgl_loader_obj_mtl.html

@gkjohnson
Copy link
Collaborator Author

Looks like with linear lighting the specular look changes quite a bit in the face especially. I'm not sure if this would be considered an issue?

BEFORE AFTER
image image

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 21, 2022

Please be aware of #7290.

Since three.js does not interpret specular maps as color maps so far (they are in fact reflectivityMap maps), it's not right to configure them with sRGBEncoding. MeshPhysicalMaterial.specularColorMap is the only exception though.

@gkjohnson
Copy link
Collaborator Author

Yes specular maps aren't adjusted here. Only the specular color is.

@mrdoob mrdoob added this to the r137 milestone Jan 21, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 21, 2022

Interesting. I always wondered why it was so plasticky...

Could you re-generate the webgl_loader_obj_mtl screenshot too?

@gkjohnson
Copy link
Collaborator Author

Could you re-generate the webgl_loader_obj_mtl screenshot too?

Hmm. I ran npm run make-screenshot -- webgl_loader_obj_mtl and the screenshot looks like this:

webgl_loader_obj_mtl

I'm running on Windows 10. Is there anything else I need to do?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 22, 2022

Does it work if you navigate into the test directory and execute npm i once? Maybe dependencies are out of date.

@gkjohnson
Copy link
Collaborator Author

Does it work if you navigate into the test directory and execute npm i once?

Thanks but still no luck. Looks the same. Could the change in texture format be causing this? When I revert the texture.encoding = sRGBEncoding the texture displays (though it's bright of course due to the renderer output encoding)

@mrdoob
Copy link
Owner

mrdoob commented Jan 24, 2022

Ah... Seems like the issue is that SwiftShader doesn't support WEBGL_compressed_texture_s3tc_srgb...

I checked out your branch and changed the puppeteer script to not be headless and I found this in devtools:

Screen Shot 2022-01-24 at 8 49 27 AM

@Mugen87 Any suggestions? Would it be better if the renderer was tolerant about this and rendered these textures wrong, better than failing and rendering black?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 24, 2022

Would it be better if the renderer was tolerant about this and rendered these textures wrong, better than failing and rendering black?

I would not do that. Displaying the uncompressed data means undefined result. What the engine currently does (reporting errors/warning) is the best.

BTW: I can generate the screenshot on my iMac without issues. This is the result:

webgl_loader_obj_mtl

@mrdoob
Copy link
Owner

mrdoob commented Jan 24, 2022

BTW: I can generate the screenshot on my iMac without issues. This is the result:

Oh... The we can merge and you can generate the new screenshot.

But we'll probably have to add an exception in the e2e script:

const exceptionList = [
'index',
'css3d_youtube', // video tag not deterministic enough
'webaudio_visualizer', // audio can't be analyzed without proper audio hook
'webgl_loader_imagebitmap', // takes too long to load?
'webgl_loader_texture_lottie', // not sure why this fails
'webgl_loader_texture_pvrtc', // not supported in CI, useless
'webgl_materials_standard_nodes', // puppeteer does not support import maps yet
'webgl_morphtargets_face', // To investigate...
'webgl_postprocessing_crossfade', // fails for some misterious reason
'webgl_raymarching_reflect', // exception for Github Actions
'webgl_test_memory2', // gives fatal error in puppeteer
'webgl_tiled_forward', // exception for Github Actions
'webgl_video_kinect', // video tag not deterministic enough
'webgl_video_panorama_equirectangular', // video tag not deterministic enough?
'webgl_worker_offscreencanvas', // in a worker, not robust
// webxr
'webxr_ar_lighting',
// webgpu
'webgpu_compute',
'webgpu_instance_uniform',
'webgpu_lights_custom',
'webgpu_lights_selective',
'webgpu_materials',
'webgpu_nodes_playground',
'webgpu_rtt',
'webgpu_sandbox',
'webgpu_skinning_points',
'webgpu_skinning'
].concat( ( process.platform === 'win32' ) ? [

@mrdoob
Copy link
Owner

mrdoob commented Jan 24, 2022

Actually, I can just photoshop/gimp it 🤓

I'll take care of this.

@mrdoob mrdoob merged commit f49819b into mrdoob:dev Jan 24, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 24, 2022

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jan 24, 2022

Actually, a better solution would be to remove the dds textures from this example. Will do that.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 24, 2022

Actually, a better solution would be to remove the dds textures from this example.

Indeed! Let's focus on S3TC in the webgl_loader_texture_dds example.

@gkjohnson gkjohnson deleted the srgb-obj branch January 25, 2022 08:22
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 26, 2022

BTW: OBJLoader has vertex color support.

buffergeometry.setAttribute( 'color', new Float32BufferAttribute( geometry.colors, 3 ) );

It seems this change is only consistent if the attribute is converted to sRGB, too.

@gkjohnson
Copy link
Collaborator Author

Oh oops -- you're right. My understanding is that the vertex colors of a geometry need to be in Linear color since they undergo no transformation before or in the shader. I'll make a PR in a bit.

@spencer-sherk-ai
Copy link

spencer-sherk-ai commented Apr 11, 2024

gkjohnson Just a little curious here for my own understanding -- why do we need to convert the colors from SRGB to linear, in order to be compatible with a renderer using SRGB encoding? This seems counter-intuitive to me ( I would think this would happen the other way around ) but probably just some knowledge gap in my understanding about the lower level of renderer color encoding.

@gkjohnson
Copy link
Collaborator Author

why do we need to convert the colors from SRGB to linear, in order to be compatible with a renderer using SRGB encoding

Only the final image is output and displayed in sRGB. All of the math being done on the GPIU happens in Linear sRGB color space.

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