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

Use same hitcircle radius for all skins #30416

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

minetoblend
Copy link
Contributor

@minetoblend minetoblend commented Oct 24, 2024

As discussed in #30012

Removes the size-difference between the selection overlay & the hitobjects in the editor by making sure that all skins as well as the selection blueprints use the same radius as stable.
For argon skin I wasn't 100% sure how this is supposed to affect sliderbodies since they have a different radius compared to circles, so I made them scale down as well, to maintain their overall appearance.

2024-10-24.19-54-44.mp4

@minetoblend
Copy link
Contributor Author

I accidentally broke the radius in HitCircleOverlapMarker so approach circles look off in the video above, should be fixed now & looks like this.
image

@bdach bdach requested a review from peppy October 25, 2024 06:15
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Oct 25, 2024
@OliBomby
Copy link
Contributor

OliBomby commented Oct 25, 2024

Why so many downvotes, are we going in the wrong direction?

Please comment why if you downvoted

@Stoppedpuma
Copy link
Contributor

Why so many downvotes, are we going in the wrong direction?

Please comment why if you downvoted

I downvoted because of the changes towards the default skins hitcircle sizes. I'm overall against continuing forward with incorrectly sized assets both from a gameplay and visual perspective. In my eyes making the default skin incorrect game-wide to solve an issue the majority of players don't care about is the wrong way of handling this. Whilst I can understand the dislike mappers feel from the blueprint being a different size, cutting short on the gameplay side where majority of the users focus will be is not correct in my opinion.

@peppy
Copy link
Member

peppy commented Oct 25, 2024

I also question the number of downvotes and would seek explanations for each person who downvoted.

cc/ @Detze @Artcens @MicrocontrollersDev @Stoppedpuma

In my eyes making the default skin incorrect game-wide

I'm assuming mean "objectively incorrect", right? The problem is, we're making this game for users that have been playing it for over a decade. And by them, what we currently have is seen as incorrect, regardless of how correct it might be.

This is a recurring theme with things we've fixed in lazer to be objectively correct, and then realised that we don't want to do that to preserve the game experience that users are expecting.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 25, 2024
@peppy
Copy link
Member

peppy commented Oct 25, 2024

@ppy/team-client requesting one more review of this. won't be merging until we get some more feedback from downvoters, but also would appreciate a second check to make sure i'm not overlooking anything important.

@Stoppedpuma
Copy link
Contributor

This is a recurring theme with things we've fixed in lazer to be objectively correct, and then realised that we don't want to do that to preserve the game experience that users are expecting.

While this is correct, a lot of those players also don't even realise something in incorrect in the first place. For example, one of the changelog videos mentioned that skins created using the 2013 default skin as a template have incorrect hitcircle sizes, I saw a few comments of people saying they didn't even notice, I've seen several skins since have the correct sizes.

what we currently have is seen as incorrect, regardless of how correct it might be.

I haven't seen any complaints about the "correct" sizes of lazer default skins besides a few pointing out that the head was larger than the slider body (which I don't recall anyone being against it), and most recently the blueprint problem.

Since lazer is supposed to fix a lot of the problems of stable, I would think that having a correctly sized default skin would be preferable as it's the first thing new players use. I can understand how this is seen as "incorrect" from a mappers stand point since the majority has been mapping with hitcircles which don't match the hitbox, yet I haven't seen any complaints about it being an issue for mappers up until the conversation about the editors blueprint.

@peppy
Copy link
Member

peppy commented Oct 25, 2024

I think you may find that this is only because mappers haven't been willing to engage with lazer until now, and it's going to become more of a contention point if left in the current state.

From another angle: having a small amount of lenience for clicking has few downsides for a user. It means clicks near the edge of circles (that happen to be right on the edge) will always be considered hits. The only downside I can see (apart from objective correctness) is that analysis of replays without an analysis overlay enabled may look confusing to someone who doesn't know about the lenience.

@Artcens
Copy link

Artcens commented Oct 25, 2024

I'm assuming mean "objectively incorrect", right? The problem is, we're making this game for users that have been playing it for over a decade. And by them, what we currently have is seen as incorrect, regardless of how correct it might be.

yes, "objectively incorrect" is my reasoning here. but, if your view on lazer mapping state is to make the stable mappers feels like home. then its not a problem.

@Artcens
Copy link

Artcens commented Oct 25, 2024

From another angle: having a small amount of lenience for clicking has few downsides for a user. It means clicks near the edge of circles (that happen to be right on the edge) will always be considered hits. The only downside I can see (apart from objective correctness) is that analysis of replays without an analysis overlay enabled may look confusing to someone who doesn't know about the lenience.

If the scaling only happen in the editor, then it should not be a problem. And if people questioning on why it registered the hit even tho it looks not, just tell them that its their skin that have smaller circle.

@OliBomby
Copy link
Contributor

If the scaling only happen in the editor, then it should not be a problem. And if people questioning on why it registered the hit even tho its look not, just tell them that its their skin that have smaller circle.

Hit objects should have the same size in the editor as in gameplay though. If the mapper can't see what the map would look like in gameplay, that would be very bad.

@Artcens
Copy link

Artcens commented Oct 25, 2024

Hit objects should have the same size in the editor as in gameplay though. If the mapper can't see what the map would look like in gameplay, that would be very bad.

What's the main concern here? peppy stated two problems and i was answering the concern that players would have.

if we want to solve two problems at the same time, we need to scale the legacy skin's hit circle to match Lazer rather than the reverse.
and I don't know if this solution even possible.

@DarkbitNetwork
Copy link

DarkbitNetwork commented Oct 25, 2024

My issue with this PR is that argon's white edge (or whatever it is called, see the image below for what I mean) is supposed to display the "true clickable area" if the hitcircle visual size is smaller than the "true clickable area" as it is the case on the sliders.

lazer argon edge highlighted

However, this PR simply reduces the argon's hitcircle size including the white edge, which removes the entire point of displaying the "true clickable area". If we're going in this direction then we might as well remove the whole white edge thing since it is no longer accurate, but it is part of the argon's visual design so I will not agree with this approach.

Comment on lines -370 to -372
private partial class DefaultSliderBody : PlaySliderBody
{
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@peppy Whatever happened here is making zero sense to me. DefaultSliderBody has been moved out to its own file - sure, that's fine - but also changed base class from PlaySliderBody to SliderBody? And a whole mess of tests just started failing?

Can we just revert e802731 completely? I don't see any gains from it. Making things "more abstract" is also not something I want to see or condone anymore.

@bdach
Copy link
Collaborator

bdach commented Oct 25, 2024

The OP is missing a type of image that I'd expect to see for ease for review, namely one such as this:

image

From a "does this change do what it says it does" standpoint, it does appear to pass the eye test. From the "do we want this change still" angle I see three options:

  • We close this outright, do nothing, and tell mappers to deal with blueprint radii being "misleading" and instead tell them that they've been mapping wrong all this time, their "spacing" is make-believe, and they should fix their skins and conception of game mechanics. (Likely to be met with torches from mappers, least effort.)
  • We merge this and tell players that the visible radius is not the clickable radius and there is invisible leniency, but mappers can still live in make-believe world of legacy default skin. (As is apparent, met with torches from some players already.)
  • We do the opposite thing of this and instead adjust all classic skins (or maybe just legacy default?) in the inverse manner such that the visible size of the circle matches the clickable size. (Likely to be met with torches from everyone.)

Not sure. I definitely wasn't expecting thumbs-down on PRing this already. I'll definitely say that I am still definitely against doing the "hey everyone can just see the radius they want to see :)" thing because having visible object radius depend on skin, for default skins is asinine. It's basic gameplay mechanics. It 500% needs to be consistent.

@MicrocontrollersDev
Copy link

MicrocontrollersDev commented Oct 25, 2024

I gave a negative reaction because I personally believe that showing an incorrect hit object size is just not good for a rhythm game, especially one that's quite heavy on aim. Ideally I believe all skins should be updated to the correct size as is done with Argon and Triangles, but I'm also well aware just how unhappy this would make a good portion of the community and that it's mostly wishful thinking. I don't really think there's a good solution here, and the direction this PR is going is probably the only way to satisfy most people.

The thumbs down was more of a "I think there should be a better solution" but short of a user toggle to choose circle size between legacy and new sizes or as a part of the classic mod, which doesn't really solve the issue of game-wide consistency or editor concerns, I have no solution to provide.

I think this PR is fine going forward, it's what people are used to and it hasn't been a source of issues in the past, I just see it as unfortunate as I believed accurate hitboxes to be a step in the right direction.

Edit: Upon reflection I am curious as to how much of the community would truly be upset if all hitcircles in skins were expanded to the correct size. It would seemingly (but not in actuality) make the gameplay easier. Would players actually be upset? It may make some maps look odd but gameplay wise it would not hurt, as opposed to lowering circle size as that would force me to be more accurate since I have no indication where the tolerance lies.

@Detze
Copy link
Contributor

Detze commented Oct 25, 2024

It's impossible to have all three of sizing consistency, visual and hitbox size match, and preservation of legacy visual hitcircle/sliderhead distances. One of them has to be given up.

If sizing consistency is given up, another possible idea would be a skin option to choose one of the two visual sizes. Resolution of all of the issues should be achievable without disapproval from any group under the third approach using such a setting. If sizing consistency cannot be given up, either hitbox-correct visuals or legacy visual distances have to be given up.

I question that precise visual hitcircle/sliderhead distances matter, because the player can change the map's CS using Difficulty Adjust, Easy, or Hard Rock anyway. So if one visual circle size has to be chosen, I'd argue there isn't much of an upside to the "118 px" size at all, whereas breaking the match between visual circle size and the correct hitbox is a significant downside. That's just my personal opinion though.

Note that it's not just the hitcircle/sliderhead that could use a match between visuals and the hitbox. It's also the sliderbody radius, because the slider re-entry hitbox is the same as the sliderhead hitbox, IIRC. This is one good reason to prefer "128 px" as the default sliderbody radius over "118 px". Currently, this is the case for the triangles skin but not the argon skin or legacy skins (as seen in the above image), nor skins in stable.

I would suggest upscaling legacy skin visual circle size (so, the third approach), and together with the sliderbody radius, for the above reason. It would be nearly identical visually to simply lowering CS by a small amount, and thus I doubt that players would disapprove of the upscaling, just like almost every player doesn't seem to notice that stable's visual circle size doesn't match the hitbox. The only concern I could see with this idea is that the circle graphics would lose a bit of quality when upscaled. To mitigate this somewhat, the upscaling for all legacy skins could be applied only in the editor, and not the gameplay, for the sought consistency (the graphical quality loss in this specific situation is a small price to pay, IMO). The default legacy skin gameplay could also be upscaled. Or it could simply not be be mitigated, with upscaling applied for all legacy skins everywhere, and users instead encouraged to remake their skins in a lazer skinning system.

I agree that mapping to legacy visual circle size is "wrong" and that spacing in such mapping is make-believe, and I personally would not want to give up the visuals and hitbox size match just to save it. That's why I downvoted the PR.

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

Successfully merging this pull request may close these issues.

9 participants