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

Loader: MaterialX #24707

Merged
merged 11 commits into from
Oct 27, 2022
Merged

Loader: MaterialX #24707

merged 11 commits into from
Oct 27, 2022

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Sep 28, 2022

Related issue: #24674

Description

Finishing the first version of MaterialX Loader...

https://raw.githack.com/sunag/three.js/dev-materialx-loader/examples/webgl_nodes_loader_materialx.html

This contribution is funded by Google

@mrdoob
Copy link
Owner

mrdoob commented Sep 28, 2022

/cc @jstone-lucasfilm

@mrdoob
Copy link
Owner

mrdoob commented Sep 29, 2022

Looking great! 🙏

Minor thing: Instead of using a dropdown, maybe it would be better to display all the materials at once by repeating the model in a grid?

@sunag sunag added this to the r146 milestone Sep 30, 2022
@jstone-lucasfilm
Copy link

This looks very promising, @sunag!

In order to make visual validation as straightforward as possible, you might consider including the san_giuseppe_bridge.hdr environment, which is used by default in the MaterialX Web Viewer example:

https://academysoftwarefoundation.github.io/MaterialX/

This would allow comparisons between the two viewing environments, with lighting environments sufficiently aligned to discern subtle visual differences. One visual difference that often comes up is slightly different interpretations of color spaces, where the color triples values in our example library are in lin_rec709 and the color images are in srgb_texture:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/resources/Materials/Examples/StandardSurface/standard_surface_carpaint.mtlx#L2
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/resources/Materials/Examples/StandardSurface/standard_surface_brass_tiled.mtlx#L5

@sunag
Copy link
Collaborator Author

sunag commented Oct 24, 2022

Really the ColorSpace conversion solved it :)

< left MaterialX Viewer, right Three.js MaterialXLoader >

image

There is a slight difference that I will have to investigate.
Maybe there are still some issues about converting from Standard Surface to glTF PBR.

@sunag sunag changed the title Loader: MaterialX (WIP) Loader: MaterialX Oct 25, 2022
@sunag sunag marked this pull request as ready for review October 25, 2022 18:19
// Original shader code from:
// https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/stdlib/genglsl/lib/mx_transform_color.glsl

export const mx_transform_color = code( `#define M_AP1_TO_REC709 mat3(1.705079555511475, -0.1297005265951157, -0.02416634373366833, -0.6242334842681885, 1.138468623161316, -0.1246141716837883, -0.0808461606502533, -0.008768022060394287, 1.148780584335327)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can actually convert this code to ShaderNode code already?


node = this.getNodeByName( 'value' );

} else if ( element === 'position' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we move this to MtlXLibrary?


node = positionLocal;

} else if ( element === 'tiledimage' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move this (and image) to MtlXLibrary, if we use a ShaderNode there and pass file similar to other arguments.

@sunag
Copy link
Collaborator Author

sunag commented Oct 26, 2022

@LeviPesin I'm thinking of reviewing this after the r146, I will add more things too... if for @Mugen87 the PR is ok to merger? It's good points but it won't affect how it works right now.

@mrdoob mrdoob merged commit 78e268f into mrdoob:dev Oct 27, 2022
@sunag sunag deleted the dev-materialx-loader branch October 27, 2022 07:02
@joshuaellis joshuaellis mentioned this pull request Oct 31, 2022
19 tasks
@sunag sunag mentioned this pull request Dec 2, 2023
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