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 20 commits into
base: dev
Choose a base branch
from
Open

feat: texture mapping movement #2722

wants to merge 20 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 c08e12e
Logs https://github.com/decentraland/unity-explorer/actions/runs/12020725287
Download Windows https://github.com/decentraland/unity-explorer/suites/31323523999/artifacts/2236061785
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/31323523999/artifacts/2236066944
Download Mac S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/texture-mapping-movement/Decentraland_macos.zip
Built on 2024-11-26T00:02:08Z

@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
public abstract void UpdateSDKTransform(ref SDKTransform sdkTransform);
public abstract void UpdateTransform(Transform transform);

public virtual void UpdateSDKTransform(ref SDKTransform sdkTransform) { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its a good opportunity to remove these three methods (UpdateSDKTransform, UpdateTransform and UpdateMaterial) from the interface.
If we continue down this path, any new thing that we want to tween (IE: bones or UI) would need to be added to the interface as virtual.
We could think of doing extensions methods for ICustomTweener. Please ping me when available and we can walk trough it together

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

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?

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