8000 Add check to detect whether mania is skinned by mcendu · Pull Request #8535 · ppy/osu · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add check to detect whether mania is skinned #8535

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
Merged
merged 6 commits into from
Apr 1, 2020
Merged

Add check to detect whether mania is skinned #8535

merged 6 commits into from
Apr 1, 2020

Conversation

mcendu
Copy link
Contributor
@mcendu mcendu commented Apr 1, 2020

Fixes #8532

@smoogipoo
Copy link
Contributor

I'm not sure about this. It's quite fragile:

  1. The user may have skinned 4K but not 7K.
  2. It also has no consideration for animations - mania-note1-0 may be the only texture for example.
  3. It's possible that the notes aren't skinned, but other components are (say, hold note body texture replaced with a flat texture because it's too distracting).

I see two possible solutions:

  1. Fallback to the classic skin. We don't have the ability for this yet, not sure how hard it'd be to implement. This should be the eventual goal.
  2. Perform the texture check per-component. I think this is probably the easiest solution, but will require giving ManiaSkinComponent probably the same amount of logic as in ManiaSkinConfigurationLookup and ManiaLegacyElement/ManiaLegacyColumnElement. This would only be a temporary measure really, since the fallbacks would be to lazer's skin, which we likely don't want for legacy skins (see point 3).

@smoogipoo
Copy link
Contributor

After reconsideration, I think this would be fine as a super temporary solution for the case that the user doesn't have any mania skin, under the condition that this checks via this.GetAnimation().

It'll have to be changed at some point to cover the other two cases I mentioned, though.

@smoogipoo
Copy link
Contributor

This also needs to not return legacy mania config values in the same cases.

smoogipoo
smoogipoo previously approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor
@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Yeah I think this is fine for now

@peppy peppy merged commit e862ca2 into ppy:master Apr 1, 2020
@mcendu mcendu deleted the mania-existence-check branch April 2, 2020 13:46
@peppy peppy mentioned this pull request Sep 6, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoogipoo smoogipoo smoogipoo approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

osu!mania is not visible when a skin is used without osu!mania components
3 participants
@mcendu @smoogipoo @peppy
0