Replace unsafe casting in `ManiaDifficultyCalculator` with generic column count calculation #30026

Givikap120 · 2024-09-27T17:33:49Z

This is a prerequisite for #29989 to work
Makes this function work with any generic IBeatmap, not only ManiaBeatmap

bdach · 2024-10-03T06:22:52Z

!diffcalc
RULESET=mania

github-actions · 2024-10-03T07:30:50Z

Target: #30026
Spreadsheet: https://docs.google.com/spreadsheets/d/1t6x_qZLE6vr9vLvg4FOEHZOCi7YrfZ4jIEmgLK8nu7I/edit

bdach · 2024-10-04T06:37:08Z

Well something ain't right here, since I'd expect the spreadsheet to be empty. And yet it's not.

Givikap120 · 2024-10-04T14:41:25Z

Well something ain't right here, since I'd expect the spreadsheet to be empty. And yet it's not.

Hmmm, very weird
I personally checked around 100 maps (including both converts and og maps) and they were matching
I will look into this

Givikap120 · 2024-10-04T15:05:05Z

Well something ain't right here, since I'd expect the spreadsheet to be empty. And yet it's not.

Checked in the latest lazer - the output is the same as this branch
This looks like problem with lazer in general

https://osu.ppy.sh/scores/2211726688

Givikap120 · 2024-10-04T15:13:08Z

@smoogipoo can you run mania sheet on master branch?

bdach · 2024-10-04T15:22:36Z

The spreadsheet generated above is against master branch...? What are you hoping to achieve with that?

Givikap120 · 2024-10-04T15:29:57Z

The spreadsheet generated above is against master branch...? What are you hoping to achieve with that?

both master lazer and master osu-tools output value the same as this branch (39pp)
when sheet claims that master value is 43pp

Givikap120 · 2024-10-04T15:56:15Z

Also, I found out that this change breaks osu-tools
Reason - LegacyBeatmapConversionDifficultyInfo.FromBeatmapInfo
This function takes ObjectCount from BeatmapInfo. This is intentionally used instead of FromBeatmap because latter will produce incorrect results as passed Beatmap is already converted, changing object count
But, in osu-tools (and potentially in some other places) - ObjectCount in BeatmapInfo is not populated, breaking the function

Text above makes me not so sure about this change anymore. The potential fix I see is passing not converted beatmap into CreateSkills function.

smoogipoo · 2024-10-07T06:30:17Z

osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs

-            var skills = CreateSkills(Beatmap, playableMods, clockRate);
+            var skills = CreateSkills(beatmap.Beatmap, playableMods, clockRate);


This is not correct - it will reference the original beatmap, not the post-mod or even mania-converted one.

smoogipoo · 2024-10-07T06:33:29Z

See review above. Current code is very intentional, and I'm not sure you'll be able to resolve the review without a total rewrite of this PR/if at all.

Givikap120 · 2024-10-07T10:15:11Z

See review above. Current code is very intentional, and I'm not sure you'll be able to resolve the review without a total rewrite of this PR/if at all.

This PR was needed for correct functioning of IDifficultyCalculatorBeatmap because it failed a cast
but if this way of fixing isn't acceptable - I can add IBeatmap IDifficultyCalculatorBeatmap.BaseBeatmap method
I just thought that this way would be better

smoogipoo · 2024-10-07T12:09:44Z

!diffcalc
RULESET=mania

smoogipoo · 2024-10-07T12:14:31Z

I think you might be able to get away with storing the IWorkingBeatmap from the constructor, local to ManiaDifficultyCalculator.

I looked back on this PR because I also don't understand why the values changed and may indicate something going wrong in osu-difficulty-calculator, which would be a must-fix for next release.

Givikap120 · 2024-10-07T12:29:11Z

I think you might be able to get away with storing the IWorkingBeatmap from the constructor, local to ManiaDifficultyCalculator.

I looked back on this PR because I also don't understand why the values changed and may indicate something going wrong in osu-difficulty-calculator, which would be a must-fix for next release.

I assume that it's because mania have merged but not deployed changes for a while now

smoogipoo · 2024-10-07T12:31:05Z

The comparison is against master, so that's not relevant.

Givikap120 · 2024-10-07T12:36:18Z

The comparison is against master, so that's not relevant.

well idk how it's comparison against master if my lazer release client show values in line with this PR instead of master
you can try downloading this score - https://osu.ppy.sh/scores/2211726688
and comparing values on lazer client to values shown in the sheet

smoogipoo · 2024-10-07T12:47:40Z

I already said I don't know why it's broken... And why I'm doing my own investigation.

I'm only telling you how things work and have worked since the very beginning with the sheet generator.

REGARDLESS of that, this PR cannot pass. It will guaranteed break at least catch with only a glance at the code.

github-actions · 2024-10-08T06:52:31Z

Target: #30026
Spreadsheet: https://docs.google.com/spreadsheets/d/1iATKETZ6UptdGBIhno0OSlhKpZeGqjKNV7VHYL6Q4pQ/edit

initial commit

0815611

pull-request-size bot added the size/S label Sep 27, 2024

Update ManiaDifficultyCalculator.cs

4ea7419

stanriders added type:code-quality area:difficulty labels Sep 30, 2024

fixed the problem via using raw beatmap

9ba28e3

smoogipoo requested changes Oct 7, 2024

View reviewed changes

smoogipoo closed this Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace unsafe casting in `ManiaDifficultyCalculator` with generic column count calculation #30026

Replace unsafe casting in `ManiaDifficultyCalculator` with generic column count calculation #30026

Givikap120 commented Sep 27, 2024

bdach commented Oct 3, 2024

github-actions bot commented Oct 3, 2024 •

edited

Loading

bdach commented Oct 4, 2024

Givikap120 commented Oct 4, 2024

Givikap120 commented Oct 4, 2024 •

edited

Loading

Givikap120 commented Oct 4, 2024

bdach commented Oct 4, 2024

Givikap120 commented Oct 4, 2024

Givikap120 commented Oct 4, 2024 •

edited

Loading

smoogipoo Oct 7, 2024 •

edited

Loading

smoogipoo commented Oct 7, 2024

Givikap120 commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

Givikap120 commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

Givikap120 commented Oct 7, 2024 •

edited

Loading

smoogipoo commented Oct 7, 2024 •

edited

Loading

github-actions bot commented Oct 8, 2024

		var skills = CreateSkills(Beatmap, playableMods, clockRate);
		var skills = CreateSkills(beatmap.Beatmap, playableMods, clockRate);

Replace unsafe casting in ManiaDifficultyCalculator with generic column count calculation #30026

Replace unsafe casting in ManiaDifficultyCalculator with generic column count calculation #30026

Conversation

Givikap120 commented Sep 27, 2024

bdach commented Oct 3, 2024

github-actions bot commented Oct 3, 2024 • edited Loading

bdach commented Oct 4, 2024

Givikap120 commented Oct 4, 2024

Givikap120 commented Oct 4, 2024 • edited Loading

Givikap120 commented Oct 4, 2024

bdach commented Oct 4, 2024

Givikap120 commented Oct 4, 2024

Givikap120 commented Oct 4, 2024 • edited Loading

smoogipoo Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

smoogipoo commented Oct 7, 2024

Givikap120 commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

Givikap120 commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

Givikap120 commented Oct 7, 2024 • edited Loading

smoogipoo commented Oct 7, 2024 • edited Loading

github-actions bot commented Oct 8, 2024

Replace unsafe casting in `ManiaDifficultyCalculator` with generic column count calculation #30026

Replace unsafe casting in `ManiaDifficultyCalculator` with generic column count calculation #30026

github-actions bot commented Oct 3, 2024 •

edited

Loading

Givikap120 commented Oct 4, 2024 •

edited

Loading

Givikap120 commented Oct 4, 2024 •

edited

Loading

smoogipoo Oct 7, 2024 •

edited

Loading

Givikap120 commented Oct 7, 2024 •

edited

Loading

smoogipoo commented Oct 7, 2024 •

edited

Loading