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

Skip initial autosize transform in CompositeDrawable #6560

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

minetoblend
Copy link

Prior discussion on discord.

Adds a SkipInitialAutoSizeTransform property to CompositeDrawable and Container to allow automatically skipping the autoSize transform when it is applied for the first time. Loosely based on how VisibilityContainer skips it's initial hide animation using didInitialHide.

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

I was hoping for a more embedded solution wherein a pending autosize transform is added at the very beginning of the first update frame and can be finished immediately by the consumer calling FinishTransforms(true) at LoadComplete. But that'll probably be complicated to implement. Not 100% sure about this though, need opinions.

Comment on lines 949 to 953
if (SkipInitialAutoSizeTransform && !didInitialAutosize)
{
FinishTransforms(false, nameof(baseSize));
didInitialAutosize = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved inside updateChildrenSizeDependencies and potentially not do the transform in the first place rather than immediately terminating it? Feels weird to have laid out flat in the main update process.

Copy link
Author

Choose a reason for hiding this comment

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

The original thought behind placing it there was that updateChildrenSizeDependencies can also get called from outside the update loop, so putting it here would make sure that the component had the chance to do a full update before finishing the transform.
I'm fine with moving it into updateChildrenSizeDependencies though.

Copy link
Author

Choose a reason for hiding this comment

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

I ended up moving it into autoSizeResizeTo instead, seemed like a more convenient place to do this to me.

peppy
peppy previously approved these changes Apr 1, 2025
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Probably fine

@bdach
Copy link
Collaborator

bdach commented Apr 2, 2025

While the problem being described here is real and has several instances of being worked around game-side, I don't like this solution and don't think it's any better than a game-side workaround. It's basically swapping a "you need to know that you have to schedule setting autosize duration/easing" to "you need to know that you have to set this one flag specific to this situation". And is there a case where you wouldn't want this flag set, even, for that matter?

The ideal solution would be to somehow have FinishTransforms() be able to finish the autosize transforms since that's what we use idiomatically to suppress initial animations these days.

Loosely based on how VisibilityContainer skips it's initial hide animation using didInitialHide.

Using VisibilityContainer is one of the worst examples to pull up in favour of this because its "initial hide" stuff is completely stupid and needs to be worked around in several instances game-side, by the way (exhibit A, exhibit B, exhibit C).

@peppy
Copy link
Member

peppy commented Apr 2, 2025

I dunno, I've mentioned elsewhere that the fact these even use transforms is wrong in my eye. Should be changed to a per-frame implementation because right now it is objectively visually wrong – each time autosize changes the duration and easing is rest causing jerkiness.

I'd be against any implementation which cements transforms as being the way forward for this.

@bdach
Copy link
Collaborator

bdach commented Apr 2, 2025

You know that'd probably work for me too as well. Just so long as it's not one flag you have to set on $thing.

As stated does this even need to be a flag? Maybe we can just force-hardcode this behaviour?

@peppy
Copy link
Member

peppy commented Apr 2, 2025

I think I could get behind that. The case where you do want it to animate initially could be handled with a schedule-content-add-later (it seems like the edge case? although if we make the change some cases where we don't want this will likely appear).

@minetoblend
Copy link
Author

minetoblend commented Apr 2, 2025

As stated does this even need to be a flag? Maybe we can just force-hardcode this behaviour?

The original thought behind making it a flag was to make sure this doesn't cause any unexpected breakage on existing compnents. Aside from that having the initial auto-size always be instant would be my preferred solution too

@minetoblend
Copy link
Author

minetoblend commented Apr 2, 2025

Should be changed to a per-frame implementation because right now it is objectively visually wrong – each time autosize changes the duration and easing is rest causing jerkiness.

Kinda goes beyond the scope of this pr but I looked into what this could look like a bit but I don't see how this would work without giving up on AutoSizeEasing, since using easing functions like this kind of relies on having a start and end value so you can lerp between them (i.e. transforms).
I'm probably massively overthinking this but the only approach that comes to mind where you can control the easing in a meaningful way is using a second-order system (good video about the topic https://youtu.be/KPoeNZZ6H4s).

I ended up hacking together a quick thing to try it out because I was curious on how well it work.
Quick explainer for the parameters:

  • Frequency: how quickly it moves towards the new value
  • Damping: if <1 it will produce a spring-like motion, >1 will slowly ease out as it approaches the target value
  • Response: if <1 it will react to changes slowly (ease-in), 1 will start to react to changes immediately, and >1 it will overshoot the target (kinda like out-back or out-elastic-half easing, as long as damping is >= 1)
2025-04-02.17-12-03.mp4

@bdach
Copy link
Collaborator

bdach commented Apr 3, 2025

The original thought behind making it a flag was to make sure this doesn't cause any unexpected breakage on existing compnents. Aside from that having the initial auto-size always be instant would be my preferred solution too

I'd honestly say it's probably fine to just remove the flag and see what breaks later.

I'm probably massively overthinking this but the only approach that comes to mind where you can control the easing in a meaningful way is using a second-order system

I'd rather not have a second completely parallel easing system existing in the game, so that one is a NAK as far as I'm personally concerned.

@minetoblend
Copy link
Author

I'd rather not have a second completely parallel easing system existing in the game, so that one is a NAK as far as I'm personally concerned.

No surprise there, I merely wanted to try it out cuz it looked interesting. I'll just remove the flag and make it the default behaviour. I'm also gonna try to look through all uses of AutoSizeDuration in lazer and see if anything looks broken to me.

@peppy
Copy link
Member

peppy commented Apr 3, 2025

without giving up on AutoSizeEasing

Yes, it would have to go, and be replaced with a single value that adjusts how the continuous damping is applied. There's no really good way of doing an easing selection for something like this.

@minetoblend
Copy link
Author

minetoblend commented Apr 3, 2025

Test failure was caused by the didInitialAutoSize flag only being set once autoSize has been applied at least once, which will also skip it if you enable AutoSize soome time after the container's creation, in which case it should animate. I changed things so the flag always gets set to true in updateChildrenSizeDependencies() regardless of if any AutoSizeAxes are set or not. Also renamed the property for better clarity about what it does.

@minetoblend minetoblend changed the title Add ability to skip initial autosize transform in CompositeDrawable Skip initial autosize transform in CompositeDrawable Apr 3, 2025
@minetoblend
Copy link
Author

minetoblend commented Apr 9, 2025

Yes, it would have to go, and be replaced with a single value that adjusts how the continuous damping is applied. There's no really good way of doing an easing selection for something like this.

Alright, shall I rewrite things to use this approach instead then? I'm a bit unsure if that's the final consensus here from the conversation above.

@peppy
Copy link
Member

peppy commented Apr 10, 2025

You could give it a try and see how the resultant code looks. But others would likely have to chime in since it's going to break the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants