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

Fix storyboard videos not accepting transforms #27967

Merged
merged 8 commits into from
Apr 23, 2024

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Apr 22, 2024

In stable, storyboard videos can actually accept all kinds of transforms.

Handling of scale is a bit special here as we need to undo relative sizing. In stable, the relevant sizing code is performed in pSprite.ScaleToScreen(), which is undone when a transform is run.

Closes #27634.

@bdach
Copy link
Collaborator

bdach commented Apr 23, 2024

In stable, storyboard videos can actually accept all kinds of transforms.

I checked, and yeah they can, so the StoryboardVideo : StoryboardSprite inheritance - which was going to be my primary concern here - does appear to make sense, but where's the actual application of the transforms? As in

diff --git a/osu.Game/Storyboards/Drawables/DrawableStoryboardVideo.cs b/osu.Game/Storyboards/Drawables/DrawableStoryboardVideo.cs
index ca2c7b774c..e8f43a79f4 100644
--- a/osu.Game/Storyboards/Drawables/DrawableStoryboardVideo.cs
+++ b/osu.Game/Storyboards/Drawables/DrawableStoryboardVideo.cs
@@ -50,6 +50,8 @@ private void load(TextureStore textureStore)
                 Origin = Anchor.Centre,
                 Alpha = 0,
             };
+
+            Video.ApplyTransforms(drawableVideo);
         }
 
         protected override void LoadComplete()

This diff as is appears to "fix" the issue, but only because the scale transform in the maps involved is basically a no-op and the only thing that "suffices to fix" is the presence of the scale transform forcing relative sizing off.

This is more apparent if you test with a different transform. Here's a modified version of one of the maps in the issue: Frums - Options.zip (change extension to .osz). It has a rotate transform and a colour transform on the video, and they do show on stable but do not on lazer if that one line above is not added.

(Interestingly for whatever reason the rotate transform is broken - it just insta-rotates to the final value rather than do the rotation across 20s? And the patch above somehow mirrors this breakage? I dunno, just gonna accept it and move on given it matches.)

osu.Game/Storyboards/Drawables/DrawableStoryboardVideo.cs Outdated Show resolved Hide resolved
{
Path = path;
StartTime = offset;
TimelineGroup.Alpha.Add(Easing.None, offset, offset, 0, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what this does given that the transforms from this aren't actually applied on the drawable, as per above. But aside from that, I do wonder how this is gonna work if the video has actual user-specified alpha transforms on it.

Copy link
Sponsor Member Author

@peppy peppy Apr 23, 2024

Choose a reason for hiding this comment

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

This can be removed I believe. The equivalent is already done in DrawableVideo.LoadComplete. Left it behind after finding the relevant fades there.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

On revisiting, I think this is required to set the correct StartTime for the final usage to generate the transforms

public double StartTime

using (drawableVideo.BeginAbsoluteSequence(Video.StartTime))
{
Schedule(() => drawableVideo.PlaybackPosition = Time.Current - Video.StartTime);
drawableVideo.FadeIn(500);
using (drawableVideo.BeginDelayedSequence(drawableVideo.Duration - 500))
drawableVideo.FadeOut(500);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well I see at least two problems here, one major one minor...

  • The major one is that on all storyboards with videos the videos appear to fade in instantly now. That second fade you linked there appears to be just dead. Feels pretty bad.
  • The minor one is that fade commands don't work on video sprites at all. This can probably be dismissed until someone inevitably drags in an example of a storyboard that uses this.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I've fixed the fade in not working. The other issue can be ignored for now, I agree.

@peppy
Copy link
Sponsor Member Author

peppy commented Apr 23, 2024

but where's the actual application of the transforms

Good catch..

(Interestingly for whatever reason the rotate transform is broken - it just insta-rotates to the final value rather than do the rotation across 20s? And the patch above somehow mirrors this breakage? I dunno, just gonna accept it and move on given it matches.)

I think your added transform is missing a piece

 R,1,0,20000,0.66 // what you had
 R,1,0,20000,0,0.66 // what you likely need for it to animate

@bdach
Copy link
Collaborator

bdach commented Apr 23, 2024

I think your added transform is missing a piece

herp derp. yep that's what it was 😅

Copy link
Collaborator

@bdach bdach 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 for now

@bdach bdach merged commit c454dd2 into ppy:master Apr 23, 2024
11 of 17 checks passed
@peppy peppy deleted the storyboard-video-transforms-and-sizing branch April 25, 2024 03:19
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.

Storyboard commands are ignored for videos
2 participants