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

Added support for texture rotation #11863

Merged
merged 8 commits into from
Aug 25, 2017
Merged

Conversation

WestLangley
Copy link
Collaborator

As proposed in #5876 (comment).

This PR is backward-compatible. It adds texture.rotation and texture.center.

Instead of passing Vector4 offsetRepeat to the shader, Matrix3 uvTransform is passed.

I chose to retain the terminology offset/repeat -- instead of using the terms translate/scale. When we increase the scale of something from 1 to 2, for example, we expect the size to increase. But when increasing the texture scale from 1 to 2, the texture actually decreases in size. So, to me, repeat seems to be a better term.

@mrdoob
Copy link
Owner

mrdoob commented Aug 5, 2017

Awesome!

@mrdoob
Copy link
Owner

mrdoob commented Aug 5, 2017

Hmm... How about one step in between?

texture.matrix = new THREE.Matrix3();
texture.matrixAutoUpdate = true;

Then, for people interested in rotation/center, they could disable matrixAutoUpdate and would now have access to the matrix that is being passed to the shader.

Your example would then be more about how to manually compose the matrix manually.

@WestLangley
Copy link
Collaborator Author

So, are you suggesting NOT adding .rotation and .center as texture properties, but instead adding .matrixAutoUpdate and .matrix?

@mrdoob
Copy link
Owner

mrdoob commented Aug 5, 2017

Yeah, as a first step. I'm afraid this could open a door to people requesting different order of operations.

@WestLangley
Copy link
Collaborator Author

Hmm... the matrix is not actually "updated". It is uniforms.uvTransform that is updated every frame.

You are proposing the user have two ways to specify the uv transform matrix -- the first is via offset/repeat; the second is via a matrix. So there needs to be a flag that specifies the method used.

That approach seems to be a bit confusing to me, but I will go with it...

You prefer texture.matrix over texture.uvTransform?

Can you suggest another term instead of texture.matrixAutoUpdate? texture.useUvTransform?

@mrdoob
Copy link
Owner

mrdoob commented Aug 5, 2017

You are proposing the user have two ways to specify the uv transform matrix -- the first is via offset/repeat; the second is via a matrix.

Yeah. I thought it would be intuitive to follow what we do with Object3D. Object3D has position, rotation and scale and we compose the matrix every frame, but the user can set matrixAutoUpdate to false and compose matrix manually for their use case.

@WestLangley
Copy link
Collaborator Author

@mrdoob OK, I implemented the API you requested.

var offset = uvScaleMap.offset;
var repeat = uvScaleMap.repeat;

uniforms.uvTransform.value.setUvTransform( offset.x, offset.y, repeat.x, repeat.y, 0, 0, 0 );
Copy link
Owner

@mrdoob mrdoob Aug 8, 2017

Choose a reason for hiding this comment

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

Maybe this is better?

if ( uvScaleMap.matrixAutoUpdate === true ) {

    var offset = uvScaleMap.offset;
    var repeat = uvScaleMap.repeat;
    uvScaleMap.matrix.setUvTransform( offset.x, offset.y, repeat.x, repeat.y, 0, 0, 0 );

}

uniforms.uvTransform.value.copy( uvScaleMap.matrix );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK... you realize, however, that there may be multiple textures, and only one will have its matrix "updated" by the renderer. The way I wrote it, texture.matrix is never modified by the renderer

Are you OK with that?

Copy link
Owner

@mrdoob mrdoob Aug 8, 2017

Choose a reason for hiding this comment

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

Yeah. I understand.
Eventually we'll be updating all and send them to the shader. I still have to study Ben's PR.

@WestLangley
Copy link
Collaborator Author

Done!

@mrdoob
Copy link
Owner

mrdoob commented Aug 8, 2017

What do you think about splitting setUVTransform() into new translate(), rotate() and scale() methods in Matrix3?

So the code could be something like this:

texture.matrix
    .identity()
    .translate( - center.x, - center.y )
    .scale( 1 / repeat.x, 1 / repeat.y )
    .rotate( rotation )
    .translate( offset.x, offset.y )
    .translate( center.x, center.y );

@WestLangley
Copy link
Collaborator Author

What do you think about splitting setUVTransform() into new translate(), rotate() and scale() methods in Matrix3?

Users naturally think in terms of what they want the texture to do. For example, "rotate counterclockwise around the center". However, the uv-transform matrix is not applied to the texture, but to the uvs of the geometry. So rotating the texture counterclockwise corresponds to a rotation of the uvs clockwise; scaling the texture down corresponds to scaling the uvs up.

This is why we use the term "repeat" -- it expresses the transform in terms of the texture, even though it is the uvs that are being transformed. It is also why I declared a positive value of rotation to be a counterclockwise rotation of the texture (consistent with the rest of the library), even though such a transform rotates the uvs in the opposite direction.

So the answer to your question is I feel your suggested approach would be confusing.

@mrdoob
Copy link
Owner

mrdoob commented Aug 8, 2017

Only advanced users that want to transform the uvs in custom ways will have to deal with this part of the api though. Most of the users will continue using texture.offset and texture.repeat.

Basically we're creating a new api for advanced users. I think it may be better to make this api intuitive for advanced users instead of trying to accommodate the current api into its logic?

A bit like BufferGeometry is more designed for advanced users...

@WestLangley
Copy link
Collaborator Author

@mrdoob I made the changes you proposed. Feedback requested...

I am thinking that texture.rotation should be reintroduced with the center of rotation hardwired to ( 0.5, 0.5 ) -- as it is with CSS div transforms. That way, even when texture.matrixAutoUpdate = true, texture rotation will "just work". We would forgo the texture.center property.

@mrdoob
Copy link
Owner

mrdoob commented Aug 9, 2017

I am thinking that texture.rotation should be reintroduced with the center of rotation hardwired to ( 0.5, 0.5 ) -- as it is with CSS div transforms.

Sounds good to me! 😊

@WestLangley
Copy link
Collaborator Author

@mrdoob Done!

@mrdoob
Copy link
Owner

mrdoob commented Aug 10, 2017

I've just realised that we transformUv() probably needs to be updated to use the matrix.

https://github.com/mrdoob/three.js/blob/dev/src/textures/Texture.js#L224-L299

transformUv() is used by the Raycaster to get the transformed uv for the intersection.

@WestLangley
Copy link
Collaborator Author

I've just realised that we transformUv() probably needs to be updated to use the matrix.

Yes, I noted that in the PR.

@WestLangley
Copy link
Collaborator Author

@mrdoob Done! I think we are good to go.

@WestLangley
Copy link
Collaborator Author

@mrdoob Whatever you want to do with this is fine...

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 21, 2017

I vote for a merge 😊

@mrdoob mrdoob merged commit fd14505 into mrdoob:dev Aug 25, 2017
@mrdoob
Copy link
Owner

mrdoob commented Aug 25, 2017

Thanks! 😊

@mrdoob
Copy link
Owner

mrdoob commented Sep 12, 2017

Hmm, I think something break here:

screen shot 2017-09-12 at 4 12 35 pm

http://rawgit.com/mrdoob/three.js/dev/examples/misc_ubiquity_test2.html

@WestLangley
Copy link
Collaborator Author

#12063 (comment)
#12063 (comment)

@pailhead
Copy link
Contributor

pailhead commented Dec 1, 2017

@WestLangley @mrdoob

In 2017 people still seem to be confused with how to manipulate textures with three.js.

From #5876 (comment):

I believe it is reasonable to assume the following maps have the same offset/repeat values:
map
specularMap
normalMap
bumpMap
alphaMap
aoMap (future)
glossMap (future)
displacementMap (future)

I disagree with this, for example you could be using a tiled, repeatable leather texture with multiple channels (map, specularMap, normalMap, glossMap) and a completely different, baked AO map.


The example from stack overflow, seems like a much better candidate for a uniform change with each draw call, then drawing different geometries, but I may not quite understand the balance.

Lets's say you scatter 100 cube meshes in your scene and scale them. You want all of them to show the brick in "world space" aka if two different sized boxes are right next to each other, they should show the brick texture of the same size. Now lets say all these boxes are sitting on a ground plane, so they have some kind of AO/light map.


Option 1 (i've seen this done a few times)

  • 100 draw calls (100x TRS applied to a "node")
  • 100 different geometries
    --- sharing 1 attribute for AO map (flattened, each face has own space in UV)
    --- 100 unique attributes for regular maps (albedo, normal, specular etc)
  • 2 textures
    --- XY repeat brick texture
    --- no-repeat AO texture
  • 2 images
    --- image of a brick (tiles)
    --- image of AO (does not tile)

In this option, we process new geometry for each texture variation. If it's not a cube but something larger, this may consume more memory, the tradeoff being less operations in the shader?


Option 2 (i've seen this have problems with duplicating the texture data)

  • 100 draw calls (100x TRS applied to a "node")
  • 1 unique cube geometry
    --- 1 attribute for AO map uvs
    --- 1 attribute for regular maps uvs
  • 101 textures
    --- 100 XY repeat textures with 100 different transforms
    --- 1 no-repeat AO texture
  • 2 images (this used to be a problem)
    --- image of a brick (tiles)
    --- image of AO (does not tile)

^ This kinda makes more sense but i'm not sure if i see the entire picture. The 100 textures are just a wrapper, they all use the same image, so technically it should be 1 texture, 1 geometry, the tradeoff being more operations in the shader?


I'm having a hard time understanding why is it ok to use uniforms to scale a mesh for example, but not the texture? Ie. we don't scale the geometry.attributes.position we scale mesh.scale or mesh.matrix but we do sort of the same thing for uvs. We don't use a uniform but reprocess the geometry.

So where is the state of texture transformations at right now?

In other words

const texA = new THREE.Texture()
texA.offset.set(Math.random(), Math.random())
const texB = new THREE.Texture()
texB.offset.set(Math.random(), Math.random())
const texC = new THREE.Texture()
texC.offset.set(Math.random(), Math.random())

const mat = new THREE.MeshPhongMaterial({
  specularMap: texA,
  map: texB,
  normalMap: texC,
})

Will give me three misaligned textures on the material? All three would use the same uv attribute (channel?) which they would transform using their matrix? If only one is used i'd really like to learn the reasoning behind this :)

I.e. how far did three.js go from:

#5876 (comment)

This is basically what three.js is doing now -- although it gets the offset/repeat from the diffuse map, and all other material textures use the setting from that map.

I always found this confusing ^. So if you populate all the maps in a material, only one .offset would be used, and only from one that's in the .map. Never understood why this setting wasn't just on the material, or perhaps material.useUvTransformFrom === 'specularMap' or something like that.

@mrdoob
Copy link
Owner

mrdoob commented Dec 2, 2017

@pailhead How about creating a new issue instead? This PR has been merged and the conversation is clear and focused.

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