-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Change Taiko Classic HD, HR and HDHR to better match stable #27136
Open
vunyunt
wants to merge
19
commits into
ppy:master
Choose a base branch
from
vunyunt:taiko-classic
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
ecc98b5
Convert maximum and minimum aspect into bindable floats
vunyunt 34e888d
Match stable's nomod timerange on classic
vunyunt c75ce34
Classic hidden
vunyunt 01a3803
Classic hardrock
vunyunt 0fe6228
Implement classic hdhr
vunyunt b53a20b
Merge branch 'ppy:master' into taiko-classic
vunyunt 46b2c12
Fix codeinspect issues
vunyunt 5495523
Fix inaccurate comments
vunyunt 92ae6ca
Merge remote-tracking branch 'upstream/master' into taiko-classic
vunyunt 79cfcc8
Change TaikoPlayfieldAdjustmentContainer parameters to be protected i…
vunyunt 98f8fa2
Fix barline appearing before the beginning of playfield
vunyunt 67e86a8
Fix codeinspect issues
vunyunt e2bd8da
Fix InspectCode issue
vunyunt 8e052fe
Fix CurrentAspect not being set & revert barline fix
vunyunt 5c28597
Merge remote-tracking branch 'upstream/master' into taiko-classic
vunyunt a88b56a
Enable masking in bar line content container
vunyunt 02c9022
Merge remote-tracking branch 'upstream/master' into taiko-classic
vunyunt 8021f6b
Merge remote-tracking branch 'upstream/master' into taiko-classic
vunyunt 6c96f79
Implement NM fade-in
vunyunt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Classic hidden
- Loading branch information
commit c75ce34f758f14448f682c138359e11921bbd1d8
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upper limit used to be 16:9, why has this been increased?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to match stable with classic mod, stable allows a wider aspect ratio range
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How was this upper limit determined though?
I'm asking because I can't find anything like this in stable source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the notes fade in at some point, and while you can't see the fade in with 16:9, you can see it with a higher aspect ratio
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean sure but that's not evidence of there being a aspect ratio lock on stable, right? That is also consistent with there not being any lock in stable, isn't it? Which we had set already but this PR removes?
As far as I can tell "if you go wider than 16:9 more is shown" means either:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have access to stable's code. But I'm guessing for stable, while there isn't an explicit aspect ratio upper bound for the playfield itself, it still effectively limits the time rage to a ~2:1 aspect ratio equivalent by hiding notes beyond that point (fading them in. Without this high sv will lose most of their difficulty since player can in theory stretch the window and have indefinitely long time range.
For stable nm, it seems to start fading notes in beyond ~2:1. 16:9 doesn't seem to be a cutoff point for stable nm.
With this PR it is done by trimming the playfield instead. This way we can reuse the trim mechanism used by classic HD, while keeping the upper bound of time range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we're getting somewhere...
I have experimentally confirmed this on a single taiko map. Graphs are here: https://www.desmos.com/calculator/cdmlh4lm3u
But that generally makes me nervous as to whether it's valid to simulate the fadeout limit by clamping aspect ratio in the first place. Clamping ratio is a hard cutoff, it doesn't emulate limited visibility of the objects as they move onto the playfield, so I don't know that this should be done? Maybe the objects should just actually be fading in instead to match stable.
Note that this affects overlapping scroll too / SVs. If the fade-in is solely in the time domain (which it appears to be on stable), then this means that a fast-moving SV-accelerated circle is going to spend more of its active time fading in on screen than a normal circle would.
@ppy/team-client would appreciate thoughts on this spectacle if you can because I'm feeling increasingly out of my depth here in making shot calls as to how this should behave
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never really noticed this previously when playing normally, but I can confirm that this is true. When SV on stable nomod is high enough, it will still be fading in even with 16:9 aspect ratio
With this insight it does seem better to actually implement the fade in (trim was used under the assumption that notes take a fixed amount of space to fade in, which has proven to be false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have access to my local branch right now, but fade in has been implemented. I'll be pushing it & posting comparison videos either tonight or some time tomorrow