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.

Sign up for GitHub

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

Jump to bottom

feat: texture mapping movement #2722

Open
wants to merge 22 commits into
base: dev
Choose a base branch
from
Open

feat: texture mapping movement #2722

wants to merge 22 commits into from

Conversation

AlejandroAlvarezMelucciDCL
Copy link
Collaborator

@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL commented Nov 5, 2024 •
edited
Loading

Initiative Notion: https://www.notion.so/decentraland/Texture-mapping-movements-ex-UV-animations-5859b7d5d35f4b54be01767bf9845849

Related PRs:
Protocol PR: decentraland/protocol#230
SDK: decentraland/js-sdk-toolchain#1028

QA TEST STEPS

  1. Download this test scene and uncompress it somewhere
  2. Enter the scene root folder with a terminal/command and run npm i and then npm run start
  3. Close the Explorer that auto-opened. Leave the scene running in the console/terminal.
  4. Download the build from this PR and connect it to the running scene using the console command according to your OS
  5. The client should open and the scene should be shown
  6. The test scene has a red button to toggle the floor tiling on and off, going from (1,1) and (8,8)
    image
  7. Check the texture animation on the cubes and plane are playing.
  8. Press the button to toggle the floor tiling

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024 •
edited
Loading

badge

Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.

Name Link
Commit 167fa9f
Logs https://github.com/decentraland/unity-explorer/actions/runs/12380221862
Download Windows https://github.com/decentraland/unity-explorer/suites/32182288327/artifacts/2334158570
Download Windows S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/texture-mapping-movement/Decentraland_windows64.zip
Download Mac https://github.com/decentraland/unity-explorer/suites/32182288327/artifacts/2334173515
Download Mac S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/texture-mapping-movement/Decentraland_macos.zip
Built on 2024-12-17T20:27:02Z

@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL added the force-build Used to trigger a build on draft PR label Nov 6, 2024
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL added force-build Used to trigger a build on draft PR and removed force-build Used to trigger a build on draft PR labels Nov 6, 2024
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL added force-build Used to trigger a build on draft PR and removed force-build Used to trigger a build on draft PR labels Nov 8, 2024
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL added force-build Used to trigger a build on draft PR and removed force-build Used to trigger a build on draft PR labels Nov 11, 2024
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL added force-build Used to trigger a build on draft PR and removed force-build Used to trigger a build on draft PR labels Nov 11, 2024
This was referenced Nov 11, 2024
Merged
Open
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL mentioned this pull request Nov 12, 2024
Closed
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL marked this pull request as ready for review November 18, 2024 01:34
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL removed do not merge force-build Used to trigger a build on draft PR labels Nov 18, 2024
fcolarich
fcolarich approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@fcolarich fcolarich left a comment

Choose a reason for hiding this comment

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

Great work!! left a small comment that you can disregard :)

DafGreco
DafGreco approved these changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@DafGreco DafGreco left a comment

Choose a reason for hiding this comment

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

Implementation checked on both platforms Windows and Macos ✅

dalkia
dalkia requested changes Nov 26, 2024
View reviewed changes
Explorer/Assets/DCL/SDKComponents/Tween/Components/CustomTweener/CustomTweener.cs Outdated Show resolved Hide resolved
Explorer/Assets/DCL/SDKComponents/Tween/Components/CustomTweener/CustomTweenerImpl.cs
@@ -88,6 +88,32 @@ public override void UpdateTransform(Transform transform)
{
transform.localRotation = currentValue;
}
}

public class TextureMoveTweener : CustomTweener<Vector2, VectorOptions>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing the change mentioned above, this is going to be just a Vector2 tweener, that can be reused in UI for example; and not hardly bound to textures

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not entirely possible, since we still depend on the PBTween mode (position, scale, rotation, textureMove) to create the custom tweener and get the values.
We could add another abstraction layer but I'm not sure it's worth it, between TextureMoveTweener and Vector2Tweener, but it conflicts with the CustomTweener implementation

Explorer/Assets/DCL/SDKComponents/Tween/Systems/TweenLoaderSystem.cs

if (pbTween.ModeCase == PBTween.ModeOneofCase.TextureMove)
{
var sdkTweenTextureComponent = new SDKTweenTextureComponent(pbTween.TextureMove.MovementType);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Im not sure this component adds value. PBTween is in the entity, you can still use it when you got to determine the texture tween type.
Please check if this component can be deleted, and you can keep the creation of tweens branching free

Explorer/Assets/DCL/SDKComponents/Tween/Tests/TweenUpdaterSystemShould.cs
@@ -30,7 +30,7 @@ public class TweenUpdaterSystemShould : UnitySystemTestBase<TweenUpdaterSystem>
public void SetUp()
{
tweneerPool = new TweenerPool();
system = new TweenUpdaterSystem(world, Substitute.For<IECSToCRDTWriter>(), tweneerPool, null, Substitute.For<SceneStateProvider>());
system = new TweenUpdaterSystem(world, Substitute.For<IECSToCRDTWriter>(), tweneerPool, Substitute.For<SceneStateProvider>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you take the opportunity and do a test on the texture mapping feature?

@AlejandroAlvarezMelucciDCL
Refactored Tweeners to abstract as much as possible the tweening conc…
1abef6d 
…ept from Transforms and Materials
@AlejandroAlvarezMelucciDCL
Merge branch 'dev' into feat/texture-mapping-movement
167fa9f 
# Conflicts:
#	Explorer/Assets/Scripts/ECS/Unity/Textures/Components/TextureComponent.cs
#	Explorer/Packages/manifest.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalkia dalkia dalkia requested changes

@DafGreco DafGreco DafGreco approved these changes

@fcolarich fcolarich fcolarich approved these changes

@mikhail-dcl mikhail-dcl Awaiting requested review from mikhail-dcl

Requested changes must be addressed to merge this pull request.

Assignees

@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL

Labels
feature a new feature
Projects
SDK
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlejandroAlvarezMelucciDCL @dalkia @fcolarich @DafGreco