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

Rewrite osu!taiko's time range computation logic to match 1:1 with stable #26781

Merged
merged 1 commit into from Jan 30, 2024

Conversation

frenzibyte
Copy link
Member

After cross-comparing against stable, I've found out that the logic in the PR above could be rewritten completely to be based on stable code rather than a linear approximation given by visual comparison (especially when dealing with different aspect ratios).

I have spent some time to get an understanding of how the logic works, so I wrote explanatory comments across the logic in hope that it speeds up the understanding process and keeps the code as close and matching with stable as possible.

stable lazer
1024x768 CleanShot 2024-01-29 at 20 51 57 CleanShot 2024-01-29 at 20 51 23
1280x1024 CleanShot 2024-01-29 at 20 55 42 CleanShot 2024-01-29 at 20 55 46
1366x768 CleanShot 2024-01-29 at 20 57 52 CleanShot 2024-01-29 at 20 57 55

The beatmap/difficulty above is https://osu.ppy.sh/beatmapsets/847323#taiko/1774604. It uses a base slider velocity of 1.4x. For extra testing, I've changed the slider velocity to 2x and made another comparison:

stable lazer
CleanShot 2024-01-29 at 21 49 45 CleanShot 2024-01-29 at 21 49 41

@frenzibyte frenzibyte added ruleset/osu!taiko area:gameplay next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Jan 29, 2024
@bdach bdach self-requested a review January 30, 2024 07:05
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.

the inline comments aren't helping at all so i'll probably just do a visual crosscheck and pretend that everything is fine

if (LockPlayfieldAspectRange.Value)
currentAspect = Math.Clamp(currentAspect, MINIMUM_ASPECT, MAXIMUM_ASPECT);

// in a game resolution of 1024x768, stable's scrolling system consists of objects being placed 600px (widthScaled - 40) away from their hit location.
Copy link
Collaborator

@bdach bdach Jan 30, 2024

Choose a reason for hiding this comment

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

stable's scrolling system consists of objects being placed 600px (widthScaled - 40) away from their hit location

no reference for this (where's the 600px figure in 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.

This is connected with the rest of the comment block about the "in length" value, in that it is all sourced from this line in stable code.

In a game resolution of 1024x768, the "scaled" resolution is 640x480 because of the aspect ratio of the game resolution. So "width scaled" is 640px, subtracting 40 from it leads to the final value of 600px, which is the scrolling distance of the object in a 640x480 space (since it's assigned to the inLength variable, see scroll code.

currentAspect = Math.Clamp(currentAspect, MINIMUM_ASPECT, MAXIMUM_ASPECT);

// in a game resolution of 1024x768, stable's scrolling system consists of objects being placed 600px (widthScaled - 40) away from their hit location.
// however, the point at which the object renders at the end of the screen is exactly x=640, but stable makes the object start moving from beyond the screen instead of the boundary point.
Copy link
Collaborator

Choose a reason for hiding this comment

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

the point at which the object renders at the end of the screen is exactly x=640, but stable makes the object start moving from beyond the screen instead of the boundary point

no reference for this

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment needs to be reworded but what I was trying to convey is that stable starts scrolling the object from X = hit target + inLength, instead of X = end of screen. See scroll code and notice the X values of the transformations.


// in a game resolution of 1024x768, stable's scrolling system consists of objects being placed 600px (widthScaled - 40) away from their hit location.
// however, the point at which the object renders at the end of the screen is exactly x=640, but stable makes the object start moving from beyond the screen instead of the boundary point.
// therefore, in lazer we have to adjust the "in length" so that it's in a 640px->160px fashion before passing it down as a "time range".
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this mean? what is "a 640px->160px fashion"?

Copy link
Member Author

Choose a reason for hiding this comment

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

As in, the inMsLength property in stable currently gives the time spent by the object moving from 760px to 160px (760px is determined by stable_hit_location + inLength, when game resolution is 1024x768).

This is bad for lazer, because 640px in stable is considered the right edge of the playfield, i.e. stable begins scrolling the object from outside of the playfield, while lazer begins scrolling the object from the right edge of the playfield instead (technically scrolls from the right edge of the HitObjectContainer).

So to compromise this, the inLength is changed to be calculated by widthScaled - stable_hit_location (distance between right edge of playfield and hit location) instead of widthScaled - 40 (stable's distance, 600px)

float widthScaled = currentAspect * stable_gamefield_height;
float inLength = widthScaled - stable_hit_location;

// also in a game resolution of 1024x768, stable makes hit objects scroll from 760px->160px at a duration of 6000ms, divided by slider velocity (i.e. at a rate of 0.1px/ms)
Copy link
Collaborator

Choose a reason for hiding this comment

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

where does 760px come from here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In a game resolution of 1024x768, the value 760px comes from 160px + 600px (where 160px is hit location, and 600px is stable's "in length").

In stable code, 760px is used specifically in this line (s.Position is set to hit location here)

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.

visual crosscheck seems fine so i'm just gonna smile and nod and go along with whatever it is that is going on in this diff

@bdach bdach merged commit f5e7697 into ppy:master Jan 30, 2024
11 of 17 checks passed
@frenzibyte frenzibyte deleted the taiko-sv-v2 branch January 30, 2024 14:03
@vunyunt
Copy link
Contributor

vunyunt commented Jan 30, 2024

For classic mod and it's combination with other mods, would it be also preferable to defer the implementation to someone with access to stable's code?

@peppy
Copy link
Sponsor Member

peppy commented Jan 30, 2024

@vunyunt to keep things moving forward I'd still suggest you PR what you have.

@vunyunt
Copy link
Contributor

vunyunt commented Jan 31, 2024

Alright, I haven't started on it yet but I can try to port #19604 over, though do keep in mind it is definitely not a logical match with stable, especially with (Classic)HDHR not fully matching even visually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:gameplay next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! ruleset/osu!taiko size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

osu!taiko slider velocity doesn't match stable
4 participants