Refactor storyboard commands structure and add framework-based transform loop support #27539
Conversation
`LoopStartTime` is now baked into each `IStoryboardCommand`.
…return custom transform sequences *sigh*
Not sure when this happened >.>
I had them shuffled around in the middle of the refactor.
…emory footprint Mutation should be done only with the methods exposed by `StoryboardCommandGroup`.
Pending actual test coverage.
|
As an update, this PR is ready for review. Would appreciate testing with any storyboards that may have potential edge cases. I've already tested thoroughly with world.execute(me) and Ayane - Nageki no Mori, and while both of them look to work correctly according to my eyesight, a second pair would be helpful. |
| public abstract void ApplyInitialValue<TDrawable>(TDrawable d) | ||
| where TDrawable : Drawable, IFlippable, IVectorScalable; | ||
|
|
||
| public abstract TransformSequence<TDrawable> ApplyTransforms<TDrawable>(TDrawable d) | ||
| where TDrawable : Drawable, IFlippable, IVectorScalable; |
There was a problem hiding this comment.
Rather than have generics here I'd prefer what #27482 did and have IDrawableStoryboardElement : IFlippable, IVectorScalable if possible. I just don't really see that much point in an extra generic type param here.
(I did initially say in that pull that that felt arbitrary but I guess this explains it better than that diff ever did.)
There was a problem hiding this comment.
It's not possible to use any interface as a generic type argument for TransformSequence because it's constrained to Drawable classes, that's why I have the generic parameter here constrained to Drawables.
We can change the TransformSequence constraint to IDrawable, but I'm not sure if the effort is worth it, so I bailed out and added a pretty local generic here.
| // For performance reasons, we need to apply the commands in chronological order. | ||
| // Not doing so will cause many functions to be interleaved, resulting in O(n^2) complexity. | ||
| IEnumerable<IStoryboardCommand> commands = Commands.AllCommands; | ||
| commands = commands.Concat(loopingGroups.SelectMany(l => l.AllCommands)); | ||
|
|
||
| foreach (var command in generated.OrderBy(g => g.StartTime)) | ||
| command.ApplyTo(drawable); | ||
| } | ||
|
|
||
| private void generateCommands<T>(List<IGeneratedCommand> resultList, IEnumerable<CommandTimeline<T>.TypedCommand> commands, | ||
| DrawablePropertyInitializer<T> initializeProperty, DrawableTransformer<T> transform, bool alwaysInitialize = true) | ||
| { | ||
| bool initialized = false; | ||
|
|
||
| foreach (var command in commands) | ||
| foreach (var command in commands.OrderBy(c => c.StartTime)) |
There was a problem hiding this comment.
This looks... expensive...
AllCommands, which is aSelectManyIterator- which is then linq-concatenated with another
SelectManyIterator - and then finally linq-sorted
Was linq overhead measured here? It looks like it would be massive. I guess this is only ran during load which makes it less of a killer, but surely this can be improved by materialising somewhere?
There was a problem hiding this comment.
I have been going quite easy on the code because it's a once-off, yeah. I will revise it.
There was a problem hiding this comment.
Normally I would agree but given that storyboards have thousands of these at a time I really would like to ensure this doesn't spiral out of control.
| public override TransformSequence<TDrawable> ApplyTransforms<TDrawable>(TDrawable d) | ||
| { | ||
| if (loopingGroup.TotalIterations == 0) | ||
| return command.ApplyTransforms(d); | ||
|
|
||
| double loopingGroupDuration = loopingGroup.Duration; | ||
| return command.ApplyTransforms(d).Loop(loopingGroupDuration - Duration, loopingGroup.TotalIterations); | ||
| } |
There was a problem hiding this comment.
@bdach if I understand what you are asking, the fix lies here specifically. normally, with my refactor but without the fix applied, this code would be a for-loop manually applying the transform until TotalIteration times (causing memory allocation issues etc.).
|
Merge conflicts are probably going to be "fun" to resolve here post-#27797. |
Large post incoming.. Outdated issueAnother thing I noticed was that easings on opacity changes on sprites don't appear during gameplay. Here's a sprite code of one of my storyboards ([Astsumei - Tenshi no Kikyou (feat. L4hee)](https://osu.ppy.sh/beatmapsets/2036657#osu/4247350)) for example. (it applies to other storyboards, but this one's the easiest to examine) ``` Sprite,Foreground,Centre,"sb/p.png",320,240 V,0,87333,,853,480 F,19,87333,88333,1,0 F,16,89999,90415,1,0 F,16,90499,90999,1,0 F,16,90999,91665,1,0 F,19,103332,103532,1,0 F,19,108665,109165,1,0 F,15,71333,72833,1,0 F,18,92665,93499,1,0 F,19,183332,186332,1,0 F,19,195665,197665,1,0 F,19,199998,201998,1,0 F,19,203332,203593,1,0 F,19,203679,203979,1,0 F,19,204080,206080,1,0 F,19,121999,123999,1,0 F,19,113999,115999,1,0 F,19,124665,134665,1,0 F,19,177998,187998,1,0 F,0,295430,296430,1,0 F,19,300763,301763,1,0 F,19,303430,304430,1,0 F,19,306096,309096,1,0 F,19,348763,351763,1,0 F,19,393130,403130,1,0 ``` |
…nt errors Stable supports these on videos already from a quick read on code (since videos are generally a subclass of sprites).
|
@Coppertine I cannot find anything odd with the beatmap you attached, I've checked on one of the sprites of the storyboard that changes opacity, and the easing is populated correctly:
If the issue happens on master as well, it should be taken to an issue thread to be tackled separately. |
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
| double earliestStartTime = TimelineGroup.StartTime; | ||
| foreach (var l in loops) | ||
| double earliestStartTime = Commands.StartTime; | ||
| foreach (var l in loopingGroups) | ||
| earliestStartTime = Math.Min(earliestStartTime, l.StartTime); |
There was a problem hiding this comment.
Previously l.StartTime referred to the start time of the loop command offset from the start time of the loop group, so the loop group start time was supposed to be added to l.StartTime in here for earliestStartTime to correctly return the start time of the command...
The behaviour is now different in this PR because l.StartTime correctly refers to the absolute start time of the command, making this code more correct.
@Coppertine Thanks for the testing but I can't see anything wrong either. If you're going to make such claims then please provide timestamps of the specific moment that looks off to you, screenshots, comparisons, anything. We cannot be expected to comb over every single minute sprite in the storyboard. |
|
Apologies, opacity changing sprites was fixed in a later commit. Edited to reflect current pr state. |
|
I dunno what to do with this pull. I tried but the size of the diff and the complexities involved make it literally unreviewable as far as my powers go. About all I have to go on is that it does appear to fix the game gobbling up memory on beatmaps with those stupid loop counts and @Coppertine's empirical testing. But having the game not die on OOM does seem like a major positive so I'm tempted to just merge this with minimal review and deal with whatever fallout happens later. @ppy/team-client objections? |
|
Yeah I can agree with that. This seems to be doing the correct thing. Given how localised storyboard code is I'm inclined to get it in. Worst case we revert or rewrite the whole thing. |
TransformSequence.Loop()#27482The need for a refactor sparked while I was reviewing #27482, a PR that intended to replace the ad-hoc transform loop logic with an actual framework-based looping transform sequence.
The PR has shown that the current structure of the storyboard commands flow was not flexible enough to have looping easily supported, for multiple reasons:
GeneratedCommand<T>to append a loop operation on the transforms, but it's not possible to return a transform sequence from the transform delegates, unless another generic parameter is added to the delegate, making code extremely awful.GeneratedCommand<T>will avoid the necessity of adding generic parameters, but at the cost of usingTransformToand tracking both the property name and property grouping (technical detail inTransforms).And generally, I did not like that everything was being served to
StoryboardSpriteas flat data, andStoryboardSpritetaking on the role of doing literally everything by itself. If I understand correctly, that shows a lack of encapsulation and polymorphism, destroying code readability as well as quality/flexibility.My refactor intends to address all of the problems above by removing the command generation logic in
ApplyTransformsand use polymorphism for transform application (instead of handling that case-by-case inStoryboardSprite.ApplyTransformsand the helper methods surrounding it). The refactor also simplifies most of the structure and mergesCommandTimelineandCommandTimelineGrouptogether (removing the iteration-based properties inCommandTimelineGroupin the process).In the process, I've already supported framework-based transform loops in this PR. I can split it to a second PR, but I'm not sure there's much reason to. If it helps reviewing and ensuring stability, I can split it out.
Feedback on the efficiency of the new structure with respect to performance would be appreciated, and/or an idea on how to make benchmarks for this. I'm not quite familiar with the performance side of things, but I hope the new structure isn't overly inefficient for some reason.
I've also done a general pass on the naming of all relevant classes to read better for me. Terms like
Timelinecombined withGroupmakes me mistake the classes for something entirely different. Open for feedback.