AcademySoftwareFoundation / MaterialX

MaterialX is an open standard for the exchange of rich material and look-development content across applications and renderers.
http://www.materialx.org/
Apache License 2.0
1.87k stars 353 forks source link

NG_place2d_vector2 generates skew result when used for transform UV texture coordinates #1021

Closed vernalchen closed 2 years ago

vernalchen commented 2 years ago

The place2d node transforms the uv coordinates in the order of -pivot, scale, rotate, translate, pivot. This will result in skew results when the rotation is not zero, while scale x and scale y has different values, which is unacceptable. Instead, the sequence of transformation should be (in the order of application): -pivot, translate, rotate, scale, pivot.

sample mtlx:

<?xml version="1.0"?>
<materialx version="1.38" colorspace="lin_rec709">
  <standard_surface name="XID_Default_A0590D2507C54822B1AC8C0DD42DF3EA" type="surfaceshader">
    <input name="base" type="float" value="1" />
    <input name="metalness" type="float" value="0" />
    <input name="base_color" type="color3" output="generic_diffuse_output" nodegraph="XID_Default_A0590D2507C54822B1AC8C0DD42DF3EA_nodegraph" />
    <input name="specular" type="float" value="0" />
    <input name="specular_roughness" type="float" value="0.7" />
    <input name="specular_IOR" type="float" value="1.5" />
    <input name="thin_walled" type="boolean" value="false" />
  </standard_surface>
  <surfacematerial name="XID_Default_A0590D2507C54822B1AC8C0DD42DF3EA_shader" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="XID_Default_A0590D2507C54822B1AC8C0DD42DF3EA" />
  </surfacematerial>
  <nodegraph name="XID_Default_A0590D2507C54822B1AC8C0DD42DF3EA_nodegraph">
    <texcoord name="texcoord1" type="vector2" />
    <place2d name="place2d" type="vector2">
      <input name="texcoord" type="vector2" nodename="texcoord1" />
      <input name="offset" type="vector2" value="0.8, 0" />
      <input name="rotate" type="float" value="-110" />
      <input name="scale" type="vector2" value="0.4, 0.2" />
      <input name="pivot" type="vector2" value="0, 0" />
    </place2d>
    <image name="generic_diffuse" type="color3">
      <input name="texcoord" type="vector2" nodename="place2d" />
      <input name="file" type="filename" value="2x1inch.png" colorspace="srgb_texture" />
      <input name="uaddressmode" type="string" value="periodic" />
      <input name="vaddressmode" type="string" value="periodic" />
    </image>
    <output name="generic_diffuse_output" type="color3" nodename="generic_diffuse" />
  </nodegraph>
</materialx>

2x1inch

Skew vs perpendicular

jstone-lucasfilm commented 2 years ago

@vernalchen You make a good case for this proposed change, and it sounds quite reasonable to me.

I'm CC'ing @bernardkwok for his thoughts, since the current implementation of place2d goes back to its origins in ShaderX (the predecessor of MaterialX ShaderGen that Autodesk contributed to the project in 2019).

ashwinbhat commented 2 years ago

@vernalchen Let's update the attached sample to use stdlib place2d and image node. @jstone-lucasfilm From MaterialX specification point of view, is there any requirement for place2d to match Usd Transform2d

vernalchen commented 2 years ago

@ashwinbhat Thanks! The sample mtlx has been updated.

kwokcb commented 2 years ago

I've checked this and this was taken from Maya but it is a consistent order of operations which matches UsdTransform2d based on the documentation you mentioned Ashwin, and also MDL (see the transform_rotation_scale_translation intrinsic description). So a skew / shear can occur.

If something else is desired I guess a different variant could be added. e.g. MDL supports transform_XYZ, but I don't see this in USD.

( Khronos has a different issue related to being top-down orientation vs bottom-up which is the MaterialX convention and uses negative numbers to flip, which is not quite matching. )

kwokcb commented 2 years ago

Perhaps as a shorter term workaround, as this is inside a Autodesk specific nodedef, you could create what you want there to match Inventor / RapidRT etc ?

kwokcb commented 2 years ago

One final note. I am guessing that what you want to a "frame" (of reference) transform (Coverage, Translate / Rotate Frame). Both are allowed in Maya in the same node. They naturally have different behaviours. The current definition is not a "frame" transform is where I'm guessing is the mismatch. image

ashwinbhat commented 2 years ago

@kwokcb we would like to propose a variant of place2d that changes the order of the operations. Would the preference be a bool input or a new place2d node.

francis-wangfr commented 2 years ago

That's OK for us if "not frame" transformation is another expected behavior. Sounds good to have a variant that works for us. After getting the answer to the preference, @vernalchen could submit a PR for this issue.

vernalchen commented 2 years ago

If the preference is a new placd2d node, shall we put it in stdlib or inside autodesk lib?

francis-wangfr commented 2 years ago

As @kwokcb mentioned, Maya supports both, it seems better to have another standard place2d node in stdlib to support "frame" translation. It looks we could propose the new node with name "place2dframe" in stdlib.

vernalchen commented 2 years ago

@kwokcb I tried to fix this issue with a new place2dframe node without modifying the original place2d node. If a bool input is prefered I could also change to that way. https://github.com/AcademySoftwareFoundation/MaterialX/pull/1027

kwokcb commented 2 years ago

Hi folks, My feeling is you don't want to create "super nodes" with booleans to switch behaviour so a place2dframe makes more sense to me. Thanks for putting up the new definition.

jstone-lucasfilm commented 2 years ago

Thanks for this original report, @vernalchen, as well as the fix in pull request #1027.