Skip to content

Change osu!taiko's scrolling time range to match stable #26681

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

Closed
wants to merge 5 commits into from
Closed

Change osu!taiko's scrolling time range to match stable #26681

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1731d7b
Match stable sv, keeping current lazer limits
vunyunt Jan 23, 2024
a46ca14
Review changes
vunyunt Jan 26, 2024
d5b08c4
Fix code inspect naming issues
vunyunt Jan 26, 2024
62e1481
Merge branch 'master' into taiko-sv
peppy Jan 26, 2024
7da92eb
Finer value adjustments to better match with stable
vunyunt Jan 28, 2024
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
20 changes: 6 additions & 14 deletions osu.Game.Rulesets.Taiko/UI/DrawableTaikoRuleset.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
using osu.Game.Rulesets.Mods;
using osu.Game.Rulesets.Objects;
using osu.Game.Rulesets.Objects.Drawables;
using osu.Game.Rulesets.Taiko.Beatmaps;
using osu.Game.Rulesets.Taiko.Objects;
using osu.Game.Rulesets.Taiko.Replays;
using osu.Game.Rulesets.Timing;
Expand Down Expand Up @@ -70,19 +69,12 @@ protected override void Update()

protected virtual double ComputeTimeRange()
{
// Taiko scrolls at a constant 100px per 1000ms. More notes become visible as the playfield is lengthened.
const float scroll_rate = 10;

// Since the time range will depend on a positional value, it is referenced to the x480 pixel space.
// Width is used because it defines how many notes fit on the playfield.
// We clamp the ratio to the maximum aspect ratio to keep scroll speed consistent on widths lower than the default.
float ratio = Math.Max(DrawSize.X / 768f, TaikoPlayfieldAdjustmentContainer.MAXIMUM_ASPECT);

// Stable internally increased the slider velocity of objects by a factor of `VELOCITY_MULTIPLIER`.
// To simulate this, we shrink the time range by that factor here.
// This, when combined with the rest of the scrolling ruleset machinery (see `MultiplierControlPoint` et al.),
// has the effect of increasing each multiplier control point's multiplier by `VELOCITY_MULTIPLIER`, ensuring parity with stable.
return (Playfield.HitObjectContainer.DrawWidth / ratio) * scroll_rate / TaikoBeatmapConverter.VELOCITY_MULTIPLIER;
float aspectRatio = Math.Clamp(
DrawWidth / DrawHeight,
TaikoPlayfieldAdjustmentContainer.MINIMUM_ASPECT,
TaikoPlayfieldAdjustmentContainer.MAXIMUM_ASPECT);
Copy link
Member

Choose a reason for hiding this comment

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

I would move this inside the helper method, especially since part of the code relies on constants from that class in itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied in a46ca14. Because it needs access to the draw width and height, I've added a ComputeTimeRange on TaikoPlayfieldAdjustmentContainer too. DrawableTaikoEditorRuleset.ComputeTimeRange seems a bit redundant now, but it's referenced in DrawableTaikoEditorRuleset, so it's kept for now. Do let me know if it would be preferable to just use the one in adjustment container instead.


return TaikoPlayfieldAdjustmentContainer.AspectRatioToTimeRange(aspectRatio);
}

protected override void UpdateAfterChildren()
Expand Down
12 changes: 12 additions & 0 deletions osu.Game.Rulesets.Taiko/UI/TaikoPlayfieldAdjustmentContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Game.Rulesets.Taiko.Beatmaps;
using osu.Game.Rulesets.UI;

namespace osu.Game.Rulesets.Taiko.UI
Expand All @@ -12,11 +13,22 @@ public partial class TaikoPlayfieldAdjustmentContainer : PlayfieldAdjustmentCont
{
private const float default_relative_height = TaikoPlayfield.DEFAULT_HEIGHT / 768;

public const float DEFAULT_ASPECT = 16f / 9f;
public const float MAXIMUM_ASPECT = 16f / 9f;
public const float MINIMUM_ASPECT = 5f / 4f;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better not to introduce terms like "DEFAULT" in public constants in the codebase, I would keep that constant in the helper method instead. Also probably best to make all these constants private, as they're no longer used anywhere outside of the class itself (see other comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, applied in a46ca14, though do note that the maximum and minimum will need to be adjustable by mods in my next pr (to match stable's wider and mod-variable time range for classic mods)


public readonly IBindable<bool> LockPlayfieldAspectRange = new BindableBool(true);

public static double AspectRatioToTimeRange(float aspectRatio)
{
const float default_time_range = 7000 / TaikoBeatmapConverter.VELOCITY_MULTIPLIER;
Copy link
Member

Choose a reason for hiding this comment

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

It should be explicitly specified that such number was taken purely by visual comparison with stable. Also, can you give further insight as to why this number is being divided by VELOCITY_MULTIPLIER?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added in a46ca14

It being divided by VELOCITY_MULTIPLIER is to signify that it already takes into account the 1.4x speed up of taiko. To quote the current code's comment:

// Stable internally increased the slider velocity of objects by a factor of `VELOCITY_MULTIPLIER`.
// To simulate this, we shrink the time range by that factor here.
// This, when combined with the rest of the scrolling ruleset machinery (see `MultiplierControlPoint` et al.),
// has the effect of increasing each multiplier control point's multiplier by `VELOCITY_MULTIPLIER`, ensuring parity with stable.

Though now that it's a constant, we could just make the default 5000 instead. Or we could include this comment here. Do let me know if one of these would be preferable


// This is the fraction of the width that should not contribute to time range.
const float ratio_compensator = 380f / 1366f;
Copy link
Member

Choose a reason for hiding this comment

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

Would name it non_playable_portion at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed in a46ca14


return (aspectRatio - ratio_compensator) / (DEFAULT_ASPECT - ratio_compensator) * default_time_range;
}

protected override void Update()
{
base.Update();
Expand Down