feat: texture mapping movement tween support #230
Conversation
Test this pull request
|
There was a problem hiding this comment.
In function of the previous PR merged, I guess this is for manipulating the offset parameter, and for the Albedo texture in case of PBR and just texture in Unlit.
The changes looks good, I'd only add more comments to know clearly the scope and remove ambiguities.
- Does it affect only albedo? Or it applies for every texture?
- Maybe texture_move is just fine, but maybe you'd want more specific naming like
texture_uv_offset_move? I don't know.
Chech this commit: a689feb The concerns you have are planned for the next PR where I'll cover the stretch goals of the initiative. |
In that case, could we add the comments that it affects albedo and the offset? If you intend to merge to |
| optional decentraland.common.TextureUnion emissive_texture = 5; // default = null | ||
| optional decentraland.common.TextureUnion bump_texture = 6; // default = null | ||
|
|
||
| optional decentraland.common.TextureUnion alpha_texture = 4; // @deprecated Alpha textures are no longer supported on PBRMaterial and UnlitMaterial.alphaTexture should be used instead. |
There was a problem hiding this comment.
the @deprecated comment works if you put it in the same line ?
You should see this message in the scene when you put the mouse over this prop, or in the typings file.
(Asking becuase i'm not sure how its being propagated)
There was a problem hiding this comment.
Thanks for sharing your concerns!
As far as I know (after reading the documentation and doing a brief research), these are just comments, they don't actually do anything and it's up to the interpreter for each language what they do with it. The generated code in the client will always have these fields.
It's actually recommended to actually remove the deprecated properties and just reserve the id numbers.
I'll let @leanmendoza provide some feedback on the subject.
There was a problem hiding this comment.
The reserve should be put after one or two minor SDK releases. (and previously communicated the breaking change). It's not a big deal because it's breaking only dead code (assuming this functionality has already passed away).
About the comment, I think both, same line and previous line, forward the comment to generated code in the SDK. It's worth to confirm.
There was a problem hiding this comment.
Btw, it's something that if it doesn't fit the forwarding needs, it's can be re-visited in the future without introducing breaking changes
There was a problem hiding this comment.
Here's the generated code in the client
| optional Vector2 offset = 4; // default = Vector2.Zero; Offset for texture positioning, only works for the texture property in PbrMaterial or UnlitMaterial. | ||
| optional Vector2 tiling = 5; // default = Vector2.One; Tiling multiplier for texture repetition, only works for the texture property in PbrMaterial or UnlitMaterial. |
There was a problem hiding this comment.
I'm not totally sure about keeping the extra info about other components here. For now, it's better to keep it than not having it at all
There was a problem hiding this comment.
The idea was to have that documentation for creators when working with alpha_texture, emissive_texture, bump_texture, since the tiling and offset properties would show up with the autocomplete.
AlejandroAlvarezMelucciDCL
deleted the
feat/texture-mapping-movement-tween-support
branch
Initiative Notion: https://www.notion.so/decentraland/Texture-mapping-movements-ex-UV-animations-5859b7d5d35f4b54be01767bf9845849
Related PRs:
E@ PR: decentraland/unity-explorer#2722
SDK PR: decentraland/js-sdk-toolchain#1028