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

Fix storyboard commands occurring before the earliest point of visibility delaying gameplay #11990

Merged
merged 4 commits into from
Mar 11, 2021
Merged

Fix storyboard commands occurring before the earliest point of visibility delaying gameplay #11990

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5b3ac7
Add failing test covering alpha commands proceeding non-alpha (but ig…
peppy Mar 9, 2021
8aaba32
Fix storyboard commands occurring before the earliest point of visibi…
peppy Mar 9, 2021
efb4a36
Fix xmldoc explaining incorrect behaviour
peppy Mar 11, 2021
023af75
Merge branch 'master' into fix-storyboard-alpha-start-time-woes
smoogipoo Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion osu.Game.Tests/Visual/Gameplay/TestSceneLeadIn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ public void TestLeadInProducesCorrectStartTime(double leadIn, double expectedSta
[TestCase(0, 0)]
[TestCase(-1000, -1000)]
[TestCase(-10000, -10000)]
public void TestStoryboardProducesCorrectStartTime(double firstStoryboardEvent, double expectedStartTime)
public void TestStoryboardProducesCorrectStartTimeSimpleAlpha(double firstStoryboardEvent, double expectedStartTime)
{
var storyboard = new Storyboard();

var sprite = new StoryboardSprite("unknown", Anchor.TopLeft, Vector2.Zero);

sprite.TimelineGroup.Alpha.Add(Easing.None, firstStoryboardEvent, firstStoryboardEvent + 500, 0, 1);

storyboard.GetLayer("Background").Add(sprite);
Expand All @@ -64,6 +65,43 @@ public void TestStoryboardProducesCorrectStartTime(double firstStoryboardEvent,
});
}

[TestCase(1000, 0, false)]
[TestCase(0, 0, false)]
[TestCase(-1000, -1000, false)]
[TestCase(-10000, -10000, false)]
[TestCase(1000, 0, true)]
[TestCase(0, 0, true)]
[TestCase(-1000, -1000, true)]
[TestCase(-10000, -10000, true)]
public void TestStoryboardProducesCorrectStartTimeFadeInAfterOtherEvents(double firstStoryboardEvent, double expectedStartTime, bool addEventToLoop)
{
var storyboard = new Storyboard();

var sprite = new StoryboardSprite("unknown", Anchor.TopLeft, Vector2.Zero);

// these should be ignored as we have an alpha visibility blocker proceeding this command.
sprite.TimelineGroup.Scale.Add(Easing.None, -20000, -18000, 0, 1);
var loopGroup = sprite.AddLoop(-20000, 50);
loopGroup.Scale.Add(Easing.None, -20000, -18000, 0, 1);

var target = addEventToLoop ? loopGroup : sprite.TimelineGroup;
target.Alpha.Add(Easing.None, firstStoryboardEvent, firstStoryboardEvent + 500, 0, 1);

// these should be ignored due to being in the future.
sprite.TimelineGroup.Alpha.Add(Easing.None, 18000, 20000, 0, 1);
loopGroup.Alpha.Add(Easing.None, 18000, 20000, 0, 1);

storyboard.GetLayer("Background").Add(sprite);

loadPlayerWithBeatmap(new TestBeatmap(new OsuRuleset().RulesetInfo), storyboard);

AddAssert($"first frame is {expectedStartTime}", () =>
{
Debug.Assert(player.FirstFrameClockTime != null);
return Precision.AlmostEquals(player.FirstFrameClockTime.Value, expectedStartTime, lenience_ms);
});
}

private void loadPlayerWithBeatmap(IBeatmap beatmap, Storyboard storyboard = null)
{
AddStep("create player", () =>
Expand Down
19 changes: 19 additions & 0 deletions osu.Game/Storyboards/CommandTimelineGroup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,30 @@ public CommandTimelineGroup()
};
}

/// <summary>
/// Returns the earliest visible time. Will be null unless this group's first <see cref="Alpha"/> command has a start value of zero.
/// </summary>
public double? EarliestDisplayedTime
{
get
{
var first = Alpha.Commands.FirstOrDefault();

return first?.StartValue == 0 ? first.StartTime : (double?)null;
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this check isn't needed, since a starting alpha command with other start values will still trigger an intro on stable.

In other words, if the starting alpha command starts with say, 1, this will behave as if there are no alpha-affecting commands, when there clearly is at least one.

Copy link
Member

Choose a reason for hiding this comment

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

I also tested when a fade command has both start and end values of 0 before any other fade commands, and it still causes an intro on stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

To confirm, an alpha command starting at 1 makes the sprite invisible before the specified time?

Copy link
Member

@Walavouchey Walavouchey Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No, an alpha command starting at 1 will make the sprite initially visible (starting from first of any of the sprite's commands), 0 to 1 is vice versa.
If it's 0 to 0, it'll be invisible but still affect intro time.

Copy link
Member

@Walavouchey Walavouchey Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I misread the question, updated above comment. Answer is still no.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay sure, but I think this example will still work correctly with my implementation (and going forward, be more correct?).

Is that fail case you mention actually in any beatmap? If not, that's stable implemented incorrectly and should be considered fine to have "fail".

Copy link
Member

Choose a reason for hiding this comment

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

I guess any map with such patterns would simply have more of their storyboarded content visible. Alright then, seems correct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing that jumped out to me in this thread, about that

If it's 0 to 0, it'll be invisible but still affect intro time.

case: are we talking about something like this?

Sprite,Background,Centre,"SB\leaf.png",320,240
 F,0,-1000,-500,0,0 // fake fade that doesn't do anything
 F,0,-500,0,0,0.5

In that case I'd expect the intro to start at -500 and not -1000, since the first command doesn't actually display the sprite, while it'd be -1000 using the current implementation... Is that something that storyboards rely on to start displaying earlier?

Copy link
Member

@Walavouchey Walavouchey Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is that something that storyboards rely on to start displaying earlier?

I ran a couple greps for "fades from 0 to 0" in my song folder and found nothing of the sort (among about 530 sets with storyboards). I can't remember any such cases either.

Note that the only use for this would be to extend the intro time earlier than any storyboard sprites are visible, which would only cause a black background at the start (or just the diff background if it wasn't removed in the storyboard). That was basically the exact issue with the storyboard mentioned in #11981 in lazer.

If we're going by the logic of "only visible sprites can affect intro time" as peppy seems to suggest, then yes, that case would contradict that. The question is if it needs to be fixed in this implementation, since I'd wager that nobody uses it based on my sample size (or would need/want to use it).

On another note, checking for instances of the other cases I pointed out earlier would probably take more effort than some greps. Although I could check this programmatically on all the maps I have, I don't find it worth it if we don't care about implementing such hypothetical storyboards 1-to-1 with stable anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an interesting case for sure, but I'd like to hope that's out of scope for the time being (until it arises that it was actually necessary for a ranked beatmap to work as expected).

One would hope that noop transforms like that would be removed by the osu! editor, or whatever scripting program the storyboarder is using.

}
}

[JsonIgnore]
public double CommandsStartTime
{
get
{
// if the first alpha command starts at zero it should be given priority over anything else.
// this is due to it creating a state where the target is not present before that time, causing any other events to not be visible.
var earliestDisplay = EarliestDisplayedTime;
if (earliestDisplay != null)
return earliestDisplay.Value;

double min = double.MaxValue;

for (int i = 0; i < timelines.Length; i++)
Expand Down
45 changes: 39 additions & 6 deletions osu.Game/Storyboards/StoryboardSprite.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,46 @@ public class StoryboardSprite : IStoryboardElement

public readonly CommandTimelineGroup TimelineGroup = new CommandTimelineGroup();

public double StartTime => Math.Min(
TimelineGroup.HasCommands ? TimelineGroup.CommandsStartTime : double.MaxValue,
loops.Any(l => l.HasCommands) ? loops.Where(l => l.HasCommands).Min(l => l.StartTime) : double.MaxValue);
public double StartTime
{
get
{
// check for presence affecting commands as an initial pass.
double earliestStartTime = TimelineGroup.EarliestDisplayedTime ?? double.MaxValue;

foreach (var l in loops)
{
if (!(l.EarliestDisplayedTime is double lEarliest))
continue;

earliestStartTime = Math.Min(earliestStartTime, lEarliest);
}

if (earliestStartTime < double.MaxValue)
return earliestStartTime;

// if an alpha-affecting command was not found, use the earliest of any command.
earliestStartTime = TimelineGroup.StartTime;

foreach (var l in loops)
earliestStartTime = Math.Min(earliestStartTime, l.StartTime);

public double EndTime => Math.Max(
TimelineGroup.HasCommands ? TimelineGroup.CommandsEndTime : double.MinValue,
loops.Any(l => l.HasCommands) ? loops.Where(l => l.HasCommands).Max(l => l.EndTime) : double.MinValue);
return earliestStartTime;
}
}

public double EndTime
{
get
{
double latestEndTime = TimelineGroup.EndTime;

foreach (var l in loops)
latestEndTime = Math.Max(latestEndTime, l.EndTime);

return latestEndTime;
}
}

public bool HasCommands => TimelineGroup.HasCommands || loops.Any(l => l.HasCommands);

Expand Down