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

[Reviewed] [Transition] Fix the animation for zoomed layers #1258

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Conversation

D8H
Copy link
Contributor

@D8H D8H commented Apr 21, 2024

Demo

Changes

  • Fix the animation for zoomed layers
  • Always keep the last frame when "Forwoard" is chosen
  • Simplify the events

Todo

  • Add a property for the smoothing radius
  • Use the shape painter color and opacity instead of the parameters
  • Use easing
    • Make 3 actions:
      • Fade in
      • Fade out
      • Fade in and out
        • with 2 durations and 2 easing and a pause

@D8H D8H added 🔄 Extension update An update for an existing extension 🔍 Reviewed extension An extension that is to be reviewed in great detail before merging. labels Apr 21, 2024
@D8H D8H self-assigned this Apr 21, 2024
@D8H D8H added this to Needs review in Extensions review via automation Apr 21, 2024
@D8H D8H changed the title [Transition] Simplify the events [Transition] Fix the animation for zoomed layers Apr 21, 2024
@D8H D8H marked this pull request as ready for review April 21, 2024 10:08
@D8H D8H requested a review from a team as a code owner April 21, 2024 10:08
@VegeTato
Copy link
Contributor

I took a look and it's amazing !
Forcing the paint to stay at the end is a good idea.
I added few things:

  • Added the extension authors (was empty).
  • Finished one of the to-do you mentioned (Use the shape painter color and opacity instead of the parameters)

Example including the extension update:
Transition.zip

@D8H
Copy link
Contributor Author

D8H commented Apr 21, 2024

  • Added the extension authors (was empty).

Are you sure it's the same user?

  • Finished one of the to-do you mentioned (Use the shape painter color and opacity instead of the parameters)

When the todo list will be done, we won't do breaking changes and we'll do everything in one update to avoid to deprecate the action several time.
This is why for now, I just want your opinion and Tristan's one so we decide together what will be done.

@VegeTato
Copy link
Contributor

  • Added the extension authors (was empty).

Are you sure it's the same user?

  • Finished one of the to-do you mentioned (Use the shape painter color and opacity instead of the parameters)

When the todo list will be done, we won't do breaking changes and we'll do everything in one update to avoid to deprecate the action several time. This is why for now, I just want your opinion and Tristan's one so we decide together what will be done.

Oh ok, in that case, everything looks perfect in my opinion.
About the author name, i think so, that's the only registered name with WestBoy

the author GitHub name is westboy31
#84

@D8H D8H changed the title [Transition] Fix the animation for zoomed layers [Reviewed] [Transition] Fix the animation for zoomed layers Apr 21, 2024
@tristanbob
Copy link
Contributor

tristanbob commented Apr 28, 2024

@D8H thanks for your work on this! I have thoroughly reviewed and tested your example.

Updates

  • Great fixes and update. The events are much simpler. Your math voodoo magic is powerful!
    image

  • I see you added two hidden properties: "Index" and "Value". The purpose of these is not obvious from their name; perhaps we should rename "Index" to "SmoothingIndex"? "Value" seems to be related to "Progress", but I don't know a better name.

  • I'm not sure how I feel about disabling the behavior when it's not active. It makes sense to reduce the CPU impact, but I worry about unintended consequences. It might confuse users if they disable the behavior and then expect their actions to be disabled. Instead, this behavior activates and deactivates itself. What do you think is right?

Todo

  • Add a property for the smoothing radius

I actually don't like smoothing, so I would set this to 0. Most of the complexity in the extension is related to this smoothing, and I don't know how much value it adds.

If I were to add new effects to this extension, I'm not sure if I would implement smoothing. Another consideration is that this would only apply to some effects. For instance, the Fade effect does not use this property, so perhaps it should be an action parameter instead.

  • Use the shape painter color and opacity instead of the parameters

We could offer start and stop values, similar to how particles and the shockwave extension work.

  • Use easing

Yes, easings would be a very good addition to this extension

  • Make 3 actions:
  • Fade in
  • Fade out
  • Fade in and out
  • with 2 durations and 2 easing and a pause

The word "fade" only applies to one of the effects.

My primary question is, how can we enable new effects to be easily added to this extension?
For example, you can see the ones I made:

Perhaps it would work best with a set of actions per effect? This way each effect can use different parameters in the action.

  • Draw one-way transition effect (Parameters: Direction, Duration, Easing, etc)
  • Draw two-way transition effect (Parameters: Start Duration, Start Easing, End duration, End Easing, etc)

@D8H
Copy link
Contributor Author

D8H commented Apr 28, 2024

I see you added two hidden properties: "Index" and "Value". The purpose of these is not obvious from their name; perhaps we should rename "Index" to "SmoothingIndex"? "Value" seems to be related to "Progress", but I don't know a better name.

The Index is like a local variable. Given that the loops are really simple I don't see the need for a long name. A longer name may make the events harder to read. A bit like when you do a for loop in JS: for (let i = 0; i < count; i++) {}.

I'm not sure how I feel about disabling the behavior when it's not active. It makes sense to reduce the CPU impact, but I worry about unintended consequences. It might confuse users if they disable the behavior and then expect their actions to be disabled. Instead, this behavior activates and deactivates itself. What do you think is right?

It's how the extension worked. I want this update not to do too much breaking changes.

I was a bit surprised by this when I looked at the events but I guess users rarely use the "enable/disable behavior" action so I guess it doesn't matter much. We could search in the forum if someone complained about it.

  • Add a property for the smoothing radius

I actually don't like smoothing, so I would set this to 0. Most of the complexity in the extension is related to this smoothing, and I don't know how much value it adds.

Looking at the comments, it was to do some kind of anti-aliasing on the circle. Now that there is a property for anti-aliasing on the shape painter, I guess we could remove the feature when we rework the extension and see if users ask for it.

  • Use the shape painter color and opacity instead of the parameters

We could offer start and stop values, similar to how particles and the shockwave extension work.

What use-cases do you see for it? I though this extension was mostly use with black or as a mask.

The word "fade" only applies to one of the effects.

I see that some YouToube video call Star Wars transitions "wipe". Would "wipe in" and "wipe out" make sense?

My primary question is, how can we enable new effects to be easily added to this extension? For example, you can see the ones I made:

Perhaps it would work best with a set of actions per effect? This way each effect can use different parameters in the action.

  • Draw one-way transition effect (Parameters: Direction, Duration, Easing, etc)

  • Draw two-way transition effect (Parameters: Start Duration, Start Easing, End duration, End Easing, etc)

Yes, I think it makes sense. So to avoid too much actions, I guess we would keep a parameter to invert the transition. I guess we could make the action asynchronous. This way users can duplicate the action to make an out and in and even add a "wait" in the middle.

@D8H D8H merged commit 50200f8 into main Apr 29, 2024
4 checks passed
Extensions review automation moved this from Needs review to Added to GDevelop Apr 29, 2024
@D8H D8H deleted the transition branch April 29, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔄 Extension update An update for an existing extension 🔍 Reviewed extension An extension that is to be reviewed in great detail before merging.
Projects
Extensions review
  
Added to GDevelop
Development

Successfully merging this pull request may close these issues.

None yet

3 participants