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

Add multiplayer option to disable score multiplier #23510

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

Conversation

timiimit
Copy link
Contributor

@timiimit timiimit commented May 12, 2023

This feature spans across osu, osu-server-spectator and osu-web projects. This should be merged only when all PRs are approved.

Changes

Preview of the change

image

How it works

When this is enabled all players and spectators see the score of everyone without score multiplier applied. When players finish playing everyone also sees the score in results without multiplier applied. Scores get submitted and imported as if they had the multiplier applied. I didn't specifically add checks for newly added ScoreInfo.IsScoreMultiplierDisabled on all places, but only where required to make this work. Maybe it would be a good idea to ensure that submitted and imported scores get calculated properly too?

Tested configurations

Multiplayer

  • 2 players (NoMod, Relax)
  • 2 players (Relax, Relax)
  • 3 players (NoMod, Relax, Relax)
  • 2 players (NoMod, Relax) + 1 spectator
  • 1 player (Relax) + 1 spectator

Solo

  • 1 player (NoMod)
  • 1 player (Relax)

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.

Mostly a visual pass with no testing yet

Comment on lines 155 to 158
/// <summary>
/// Should generally not be used to change multiplier. Use settter only to override default way of computing multiplier.
/// </summary>
public double ScoreMultiplier { get; set; } = 1;
Copy link
Collaborator

@bdach bdach May 14, 2023

Choose a reason for hiding this comment

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

I'm not sure how to feel about this property being the sole hinge point of this feature but my initial thoughts are "this is not good". If you have to preface a property with "should generally not be used" it should be a big warning sign that maybe things are not entirely structured right. Anything could wipe this change and suddenly all sorts of weird stuff can happen.

If I were to provide an alternative, extensible way of making this work: have a

public Func<IEnumerable<Mod>, double> ScoreMultiplierCalculator;

field. Initialise it to the default implementation of accumulating ScoreMultiplier. Override it to

scoreProcessor.ScoreMultiplierCalculator = _ => 1;

in the pathways you need that in, and there. No more fear of something funny happening with that. (Unless something else starts messing with the calculator, but at that point, we have bigger problems.)

I guess you could also do a boolean ApplyMultipliers flag or similar if needed instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

@smoogipoo
Copy link
Contributor

Is there an issue or discussion that this PR should be linked to? When would this option be used?

@bdach
Copy link
Collaborator

bdach commented May 15, 2023

See #23452.

Mind you, I'm not sure if we want this yet either, but there is the cause of this entire series.

timiimit and others added 2 commits May 15, 2023 09:42
@timiimit
Copy link
Contributor Author

timiimit commented May 15, 2023

As you suggested i added ScoreMultiplierCalculator and then expanded on the idea. I made it a generic method that can compute multiplier from ScoreInfo. I also added the calculator to ScoreInfo so that score itself can determine how multiplier is computed.

I added a static field with default way of computing it to ScoreInfo. Perhaps calculator should have a class of its own?

Mind you, I'm not sure if we want this yet either,...

I'll stop working on it, if it is not desirable. but i have been playing multi on lazer for whole year almost each day and this feature would be really useful constantly. There are people that play multi with conversion mods, HD only, HR only, fun mods, very diverse playstyles and no one really wants to compete with mods. As an example there is someone that plays AR10.6 with DA mod (x0.5 multiplier) almost 100% of the time and sometimes just for the sake of getting comparable scores everyone else also picks DA with no changes applied.

@peppy
Copy link
Member

peppy commented May 17, 2023

In my opinion, "Disable mod score multiplier" should be rephrased to "Display scores without mod multipliers". As it is now, players might get the impression that they can submit scores without multipliers by using this option in multiplayer. It should be made more clear that this option is only a visual change.

This makes no sense. In the context of a multiplayer game, it's not a visual-only change. It changes the outcome of the room / tournament / whatever.

I would write this as "No mod multipliers" or similar.

@monochrome22
Copy link

The option doesn't actually disable mod multipliers, it just shows what the scores would look like if they didn't have multipliers applied, they are still being submitted with multipliers applied, that's why I said it's a visual change. It's true that in the context of the lobby, it's not a visual change, but in the context of the entire game, it is a visual change.
If you feel that players misinterpreting the functionality of the option is not a concern, then feel free to use "No mod multipliers".

@timiimit
Copy link
Contributor Author

I agree that "No mod multipliers" would be better, since this option is in the context of multi.
I suppose initially some may think that scores would submit as displayed, but its probably just going to be one of the things that people will learn eventually.

@peppy
Copy link
Member

peppy commented May 25, 2023

I suppose initially some may think that scores would submit as displayed, but its probably just going to be one of the things that people will learn eventually.

I mean, right now multiplayer scores don't "submit" for solo play. They are isolated to the multiplayer system.

@peppy
Copy link
Member

peppy commented May 25, 2023

As a heads-up, we should probably hold off merging this until the score v2 changes are applied (#23638), as I foresee conflicts and would rather that PR has precedence.

@timiimit
Copy link
Contributor Author

timiimit commented Jun 1, 2023

Since ScoreV2 is not recalculatable, I still need to make it calculate score with and without multiplier.

@timiimit
Copy link
Contributor Author

timiimit commented Jun 1, 2023

Actually, it seems that it is now possible to just not use the multiplier since it is applied at the end of calculation.

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.

Aside from the numerous test failures and failing code quality inspections, this implementation feels much worse than previously

@@ -49,7 +50,7 @@ public partial class SpectatorScoreProcessor : Component
/// </summary>
public IReadOnlyList<Mod> Mods => scoreInfo?.Mods ?? Array.Empty<Mod>();

public Func<ScoringMode, long> GetDisplayScore => mode => scoreInfo?.GetDisplayScore(mode) ?? 0;
public Func<ScoringMode, bool, long> GetDisplayScore => (mode, noScoreMultiplier) => scoreInfo?.GetDisplayScore(mode, noScoreMultiplier) ?? 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a bit of a dislike for Func<>s that have bools inside with no real clear meaning. I would propose declaring a named delegate type for this and properly naming the parameters of said delegate to describe them without needing to look up a random implementation to find the lambda param name.

Comment on lines 23 to 28
if (noScoreMultiplier)
{
// This might result in a minor inaccuracy of score (+-1 point) because the passed in score is already rounded.
// This shouldn't be much of a problem because noScoreMultiplier is only a visual effect meant for fun.
score = (long)Math.Round(score / scoreMultiplier);
}
Copy link
Collaborator

@bdach bdach Jun 3, 2023

Choose a reason for hiding this comment

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

Having to pass scoreMultiplier and noScoreMultiplier and having to undo an already-applied multiplier feels completely backwards. Much more so that this comment states that the undo cannot be done accurately due to rounding applied.

Copy link
Contributor Author

@timiimit timiimit Jun 7, 2023

Choose a reason for hiding this comment

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

I don't think having a minor inaccuracy is problematic. This feature implements only a visual difference in scores. Having this feature coded alongside ScoringMode as final step in calculating score that is displayed makes sense to me. Classic scoring is also just a fake front for users anyway, since internally calculations are done the same way as standardised scoring.

I made ScoreProcessor calculate proper score, but ScoreInfo still requires score multiplier to be unapplied.

Do you think it be better to ditch GetDisplayScore?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it be better to ditch GetDisplayScore?

I don't know how to answer this question because I don't know why the implementation has swung in this entire different direction. The initial version wherein the application of the multipliers (or lack thereof) was coordinated by ScoreProcessor and MultiplayerPlayer was miles better because it did not weirdly couple components like what this new variant is doing. Is there a constraint that makes that old implementation unviable that I am missing here?

Copy link
Contributor Author

@timiimit timiimit Jun 9, 2023

Choose a reason for hiding this comment

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

I forgot what the reason was, so I tried to work on previous implementation again here and made it functional. Actually, I probably didn't think enough about what continuing with old implementation would require. ScoreV2 update made all code unfamiliar and it seemed too hard to continue so I switched up my approach. In reality I only needed to add a couple of lines in Player and PlaylistResultsScreen...

The actual reason for new implementation was that I couldn't figure out a way to recalculate score with ScoreProcessor anymore. In linked branch I still can't recalculate when score is retrieved from server which is done on the results screen. Before ScoreV2 update, ScoreProcessor.CalculateScore() could return score but this method is now gone. This makes implementation still suffer from unappyling score multiplier in PlaylistResultsScreen.

Let me know if I should continue with linked branch.

osu.Game/Scoring/ScoreManager.cs Outdated Show resolved Hide resolved
osu.Game/Scoring/ScoreManager.cs Outdated Show resolved Hide resolved
@timiimit
Copy link
Contributor Author

timiimit commented Jun 7, 2023

Score retrieved from ScoreProcessor is now handled properly without undoing multiplier, but ScoreInfo only has final long TotalScore with ScoreMultiplier applied so exact value from before ScoreMultiplier was applied is impossible to get because of rounding.

ScoreInfo.GetDisplayScore is used to see others' live scores and on results screen. I will try to make it so that TotalScore in FrameDataBundle contains score without score multiplier.

osu.Game/Online/Multiplayer/MultiplayerClient.cs Outdated Show resolved Hide resolved
osu.Game/Scoring/ScoreInfo.cs Outdated Show resolved Hide resolved
@timiimit timiimit force-pushed the multi-score-multiplier-option branch from 633a5a8 to 2b4c302 Compare June 17, 2023 20:45
@timiimit
Copy link
Contributor Author

I reset branch to commit de6a51d0d9f4b225c89af1b080c6c878bca794b1 and then merged with branch mentioned here.

@bdach
Copy link
Collaborator

bdach commented Jun 18, 2023

I looked at this diff again and saw things being put in places where I don't think they should be (the ScoreMultiplierCalculator thing in ScoreInfo - why is it there at all? - and in PlayerConfiguration - does it need to be there? can't it be added manually in the multiplayer/spectator players themselves? does every single player need to be aware of this flag?) - none of which were present in the initial implementation that I was most fine with.

I don't know. I give up. Need someone else to look at this diff with a fresh outlook because I don't know what's happening here and I don't know whether I'm seeing actual problems at this stage.

@ReinIsNOTaDev
Copy link

is this pr dead?

@bdach
Copy link
Collaborator

bdach commented Jul 23, 2024

Probably. It needs to have conflicts resolved, and considerations made for things that we did to get lazer leaderboards live - at the least. And I didn't like the code quality here before either, so this is likely in need of a full rewrite if we want it to go forward.

@timiimit
Copy link
Contributor Author

I kind of stopped working on this, as I no longer liked the direction I originally took.

I thought it would be better if each individual was able to choose whether to see scores with or without multiplier instead of having room settings checkbox. This would make each player happy because they would see scores in their preferred way. But the downside is that people would be seeing scores in two different ways, which would surely be confusing. Especially where this option would change the order of players in final results screen, so there could be 2 distinct winners based on which option you use. So something would have to make it visually apparent as to why scores appear different among players.

I suppose this already is a similar problem as with classic and standard scoring. But there you can usually tell who has classic enabled because scores are so much higher (and I assume order of scores is preserved).

I also think I saw in PRs or heard from somewhere that score & score multiplier are now separated internally. And that makes it easier to now implement this.

@bdach
Copy link
Collaborator

bdach commented Jul 25, 2024

I thought it would be better if each individual was able to choose whether to see scores with or without multiplier instead of having room settings checkbox

Definitely not happening.

I also think I saw in PRs or heard from somewhere that score & score multiplier are now separated internally

We're not there yet but this sort of thing is happening to facilitate future mod multiplier rebalances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants