Skip to content

Commit

Permalink
MeshStandardMaterial: metalnessMap now modulates metalness
Browse files Browse the repository at this point in the history
  • Loading branch information
@WestLangley
WestLangley committed Nov 13, 2015
1 parent 9d64683 commit e7b3717
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/renderers/shaders/ShaderChunk/metalnessmap_fragment.glsl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ float metalnessFactor = metalness;
#ifdef USE_METALNESSMAP

vec4 texelMetalness = texture2D( metalnessMap, vUv );
metalnessFactor = texelMetalness.r; // note equality, not *=
metalnessFactor *= texelMetalness.r;

#endif

7 comments on commit e7b3717

@erich666
Copy link
Contributor

Choose a reason for hiding this comment

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

I just left a comment about roughnessMap doing this same "*=" type of operation. Why have the metalnessMap modulate (multiply) the metalness of the object? This seems not particularly useful. For example, I could set metalnessFactor to 0.5, then texelMetalness could never make the final metalnessFactor more than 0.5 - how is that useful? By default metalnessFactor is 1.0 before the texture is applied, which is good. But setting this value lower (or higher) than 1.0 seems an odd control - why bias things by making it have less metalness by setting a scalar?

As I mentioned in my roughness comments, if you really do think this is useful, vs. "because we can", then you should also add a metalnessOffset value,

metalnessFactor *= texelMetalness.r + metalnessFactor;

In this way the metalness texture could then be given an arbitrary range it can modify, such as 0.4 to 0.7 metalness, instead of the current design, which says that only the upper bound of the range, i.e., [0.0, metalness], can be set, and the lower bound of 0.0 cannot be set.

@mrdoob
Copy link
Owner

@mrdoob mrdoob commented on e7b3717 Aug 9, 2016

Choose a reason for hiding this comment

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

/ping @WestLangley @bhouston

@WestLangley
Copy link
Collaborator Author

@WestLangley WestLangley commented on e7b3717 Aug 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@erich666 Thank you for the suggestion. Yes, as you can see from the various threads, we have been back and forth on this one.

I would have no problem with metalness and roughness scale/bias parameters, so that the metalnessMap and roughnessMap can be remapped to an interval.

roughness <= bias + scale * roughness

Would you mind filing an enhancement request so the new feature and API can be discussed?

@bhouston
Copy link
Contributor

@bhouston bhouston commented on e7b3717 Aug 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@erich666 I completely understand the limitation is problematic, I ran into it as well. But I am not happy with the proposed solution, it isn't general enough.

@WestLangley I agree with your formulation that it is a more general solution. I implemented exactly this solution in this PR from March 2016 (with the exception that I added the ability to independently invert the map):

#8278

I implemented it this way with a scale and offset (another name for bias) and inverse:

https://github.com/mrdoob/three.js/pull/8278/files#diff-cea5a42c1c15fd23dde252730d82e223R43

We maintain a separate ThreeJS fork primarily for this PR because this PR's ability to have bias and scale for each quantity as well as flexible UVs sets for each map is incredibly valuable. There are a ton of people that endorsed that PR as well. The way I implemented it in this PR is that it is fully backwards compatible as well.

@bhouston
Copy link
Contributor

Choose a reason for hiding this comment

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

@erich666 here is demo of bias/offset/inverse with a metallic material: http://ci.threejs.org/api/pullrequests/8278/examples/webgl_materials_slots.html

@erich666
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! In my opinion it feels "heavy" to add a roughTexelScale, as "roughness" itself could act as the scale. But, your call, as roughTexelOffset would apply to only the texture, not the "untextured" roughness (i.e., it would be added to the roughness texture, not the roughness value when texturing is off).

That is,

if (textured) { finalRoughness = texture * roughness + roughnessOffset; // may want to clamp to [0,1] - I would } else { finalRoughness = roughness; }

The "roughness" on the second line could be roughnessScale, but it seems like an extra variable. That said, I can see the case for a separate value - your call. Also note you want to clamp values to be 0.0 to 1.0 range.

@bhouston
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I guess I want to unify offset and scale/bias for all maps, because they all have this limitation, not just roughness. I unified the old bumpScale with bumpTexelScale here: https://github.com/mrdoob/three.js/pull/8278/files#diff-45d8715792789df6c07e4eef373d74fbR188 But that was because bumpScale is not used without a bumpMap, but as you said roughness can be.

I wonder if the solution isn't just to unify roughness with roughTexelScale, (and metalness with metalTexelScale), but allow for the end result to be calculated whether or not there is a texture map for roughness. That may be a better unification strategy, than what I have done now. Making that change is just a few lines of code in that other PR.

That said, that PR never got merged, so maybe this discussion is moot....

Please sign in to comment.