General Touchscreen Rebalance #28396

BenZeng04 · 2024-06-05T04:33:19Z

This is a refactored version of https://github.com/TDRework/osu/tree/touchscreen-rebalance with significantly reduced runtime.

The proposal overhauls the touchscreen star-rating system, replacing the existing flat exponential to aim star rating with a probabilistic estimation of which hand is used to hit each individual object, maintaining a list of the most likely sequences while calculating difficulty.

All the changes in this PR (and in the previous version) are outlined in https://docs.google.com/document/d/1LWP7W5A5pghMpfXY0cj1v-ODjZsApe-MP2n7PvPOjYs/edit#heading=h.2910458agvn1.

tsunyoku

These are my first thoughts upon just reading the code without considering the values or actual ideas - I'll get to a more thorough review soon when I have more opportunity to properly stomach what is going on here. However, so far I just feel iffy about a lot of the naming and organisational choices.

I'm not sure how I feel about the "raw touch" and "touch" naming because at first glance - what is the difference?

tsunyoku · 2024-06-05T09:50:18Z

osu.Game.Rulesets.Osu/Difficulty/OsuDifficultyCalculator.cs

        protected override Skill[] CreateSkills(IBeatmap beatmap, Mod[] mods, double clockRate)
        {
-            var skills = new List<Skill>
+            List<Skill> skills;
+
+            if (mods.Any(h => h is OsuModTouchDevice))
            {
-                new Aim(mods, true),
-                new Aim(mods, false),
-                new Speed(mods)
-            };
+                skills = new List<Skill>
+                {
+                    new TouchAim(mods, clockRate, true),
+                    new TouchAim(mods, clockRate, false),
+                    new TouchSpeed(mods, clockRate),
+                    new Aim(mods, true),
+                };
+            }
+            else
+            {
+                skills = new List<Skill>
+                {
+                    new Aim(mods, true),
+                    new Aim(mods, false),
+                    new Speed(mods),
+                };
+            }


this part does really make me nervous about the index-based access of skills. I've mentioned this in the past but has never really been something that strikes me as "we really need to address this now" but with conditionally existing skills I think this is going to become very confusing. I think it might be a good idea to create some record/class containing all of the skills with nullables where necessary so it's less ominous or prone to errors.

I think the first thing we can do quickly is replace indexing with simple skills.First(x=> x is Speed) to at least make it less confusing to look at

I agree that this is going to be more confusing and I'm aware that index-based access of skills have been touched on before. The reason that I did this is to improve runtime, as yeahbennou mentioned to me that pre-refactor runtime is horrible, and this is one of the causes of it.

tsunyoku · 2024-06-05T09:51:24Z

osu.Game.Rulesets.Osu/Difficulty/Evaluators/SpeedEvaluator.cs

@@ -22,7 +22,7 @@ public static class SpeedEvaluator
        /// <item><description>and how easily they can be cheesed.</description></item>
        /// </list>
        /// </summary>
-        public static double EvaluateDifficultyOf(DifficultyHitObject current)
+        public static double EvaluateDifficultyOf(DifficultyHitObject current, bool singletapped)


I think the naming of singletapped could be reconsidered to seem a little less generic since it really only applies to touch screen while the speed evaluator is used for non-TD too

Definitely - I took this directly from pre-refactor. Open to suggestions.

Renamed this to tappedWithTouch for now in the latest commit. It seems more accurate.

tsunyoku · 2024-06-05T09:55:04Z

osu.Game.Rulesets.Osu/Difficulty/Skills/Touch/RawTouchSkill.cs

+
+        protected abstract double StrainValueOf(OsuDifficultyHitObject current);
+
+        protected abstract double StrainValueIf(OsuDifficultyHitObject simulated, TouchHand currentHand, TouchHand lastHand);


What does "if" even mean here?

It means that the object may potentially be hit with a different hand.

The code is severely underdocumented right now, so any confusion regarding the code is expected. I will provide documentation when I can.

tsunyoku · 2024-06-05T09:55:57Z

osu.Game.Rulesets.Osu/Difficulty/Skills/Touch/RawTouchAim.cs

+        private RawTouchAim(RawTouchAim copy)
+            : base(copy)
+        {
+            withSliders = copy.withSliders;
+        }


Do we really need all this deep clone stuff? Can't you just new RawTouchAim(clockRate, withSliders) since you have access to the existing skill anyways?

See RawTouchSkill's copy constructor.

tsunyoku · 2024-06-05T09:56:35Z

osu.Game.Rulesets.Osu/Difficulty/Skills/Touch/RawTouchSkill.cs

+            LastRightDifficultyObjects = new List<DifficultyHitObject>();
+        }
+
+        protected RawTouchSkill(RawTouchSkill copy)


Again I really don't like this design of taking in an existing object to clone from it..

tsunyoku · 2024-06-05T09:57:30Z

osu.Game.Rulesets.Osu/Difficulty/Skills/Touch/RawTouchSkill.cs

+
+        protected abstract double StrainValueIf(OsuDifficultyHitObject simulated, TouchHand currentHand, TouchHand lastHand);
+
+        public abstract RawTouchSkill DeepClone();


I feel like I already added enough comments about this at this point but I just really don't like this. What do we actually need deep cloning for?

tsunyoku · 2024-06-05T09:58:13Z

osu.Game.Rulesets.Osu/Difficulty/Skills/Touch/TouchAim.cs

+        protected override RawTouchSkill[] GetRawSkills() => new RawTouchSkill[]
+        {
+            new RawTouchAim(clockRate, withSliders),
+            new RawTouchSpeed(clockRate),
+        };


I'm not sure if it's the right place to put these "raw" skills here... generally speaking I'm having a really hard time trying to understand what the hell is going on

This GetRawSkills exists in a few places and I don't know how I feel about a "touch aim" skill containing "raw" skills for "touch aim" and "touch speed" and vice versa wherever it's relevant

Yes, the "raw" skills part is the part that I'm most concerned about. It was quite difficult to come up with a good structure as this is the part where aim and speed are mixed together. I'm open to suggestions.

tsunyoku · 2024-06-05T09:58:42Z

osu.Game.Rulesets.Osu/Difficulty/Skills/Touch/TouchProbability.cs

+        public TouchProbability(TouchProbability copy)
+        {
+            Probability = copy.Probability;
+            Skills = new RawTouchSkill[copy.Skills.Length];
+
+            for (int i = 0; i < Skills.Length; i++)
+                Skills[i] = copy.Skills[i].DeepClone();
+        }


…ect` methods

Givikap120 · 2024-06-05T12:03:20Z

I feel like this PR heavily needs general pp refactoring first
Removing independent skills system and implementing single Skills class where one skill can depend on another
Cuz without this the PR looks very messy, with all those copies

Dragging is conceptually the same as hitting the object with the last hand. Using the current DifficultyHitObject is inaccurate, since the lastLast note you are aiming from is more correctly reflected from a simulated object.

Lowered bonuses for balancing, since this change led to massive buffs.

…han `OsuDifficultyHitObject`

Removed strain-based decay of bonus and scale bonus to only non-sliders to prevent Oshama Scramble from being worth more with TD than without. Reduce bonuses overall for balancing.

Rian8337 added 3 commits June 4, 2024 19:39

Initial work of refactoring yeahbennou's touchscreen rework

7382553

Remove logging

f9f6efb

Fix code quality

a2cd9b9

pull-request-size bot added the size/XL label Jun 5, 2024

bdach added ruleset/osu! area:difficulty labels Jun 5, 2024

bdach requested a review from a team June 5, 2024 07:05

Mark lastHand in RawTouchSkill as protected

5bf1e6a

tsunyoku reviewed Jun 5, 2024

View reviewed changes

Rian8337 added 2 commits June 5, 2024 18:04

Make object histories private and only accessible through `GetLastObj…

3c13564

…ect` methods

Rename updateHistory object parameter name for better clearance

e74adb0

bdach mentioned this pull request Jun 5, 2024

Touchscreen General Rebalance #28395

Closed

BenZeng04 and others added 17 commits June 6, 2024 15:36

Use simulated object and bonus for dragging

13eb8d6

Dragging is conceptually the same as hitting the object with the last hand. Using the current DifficultyHitObject is inaccurate, since the lastLast note you are aiming from is more correctly reflected from a simulated object.

Rename singletapped to tappedWithTouch

9e3a248

Adjust constants and hand-coordination bonuses

c921b88

Use last non-drag hand for simulating drag objects

7b0717a

Lowered bonuses for balancing, since this change led to massive buffs.

Mark setter for LastNonDragHand as private

8247263

Scale FL by pp ratio instead of SR ratio

3a06e0f

Use base bonus multiplier when computing aim bonuses

b025fce

Reorganize variables in computing speed strain

856925c

Move createSimulatedSwapObject in RawTouchAim to RawTouchSkill

93f5f05

Mark clockRate in RawTouchSkill as private

354c18b

Use LastNonDragHand when getting other hand for dragging

b8367bd

Change object simulation methods to accept an OsuHitObject rather t…

eab4aea

…han `OsuDifficultyHitObject`

Document object simulation methods

194d082

Move relevant hand logic inline to its usage

05cf270

Remove unused directive

518a93a

Fix missing cast

66731af

Document GetOtherHand

72bb030

Rian8337 and others added 15 commits June 7, 2024 10:46

Document StrainValueIf

cd7a1ee

Extract last object to a variable when simulating an object

61fe914

Inline angle calculation instead of simulating an object

3a7aa38

Account for sliders in GetSwapAngle

96e2324

Fixed FL using incorrect index for TD

a67a9f5

General rebalancing of constants

5055084

Removed strain-based decay of bonus and scale bonus to only non-sliders to prevent Oshama Scramble from being worth more with TD than without. Reduce bonuses overall for balancing.

Move GetSwapAngle to RawTouchAim

b06fb54

Account for dragging when retrieving past objects

dd00e89

Document GetLastObjects and GetLastDIfficultyObjects

3bc72e0

Document TouchSkill

77f215b

Rename RawTouchSkills to TouchHandSequenceSkills

fdee519

Document TouchHandSequenceSkills

9e725c3

Rename TouchProbability to TouchHandSequenceProbability

f7b8436

Document TouchHandSequenceProbability

f4eead4

Document LastHand and LastNonDragHand

1a5dc0e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General Touchscreen Rebalance #28396

General Touchscreen Rebalance #28396

BenZeng04 commented Jun 5, 2024

tsunyoku left a comment

tsunyoku Jun 5, 2024 •

edited

Loading

stanriders Jun 5, 2024

Rian8337 Jun 5, 2024

tsunyoku Jun 5, 2024

Rian8337 Jun 5, 2024

BenZeng04 Jun 6, 2024

tsunyoku Jun 5, 2024

Rian8337 Jun 5, 2024

tsunyoku Jun 5, 2024

Rian8337 Jun 5, 2024

tsunyoku Jun 5, 2024

tsunyoku Jun 5, 2024

tsunyoku Jun 5, 2024

Rian8337 Jun 5, 2024 •

edited

Loading

tsunyoku Jun 5, 2024

Givikap120 commented Jun 5, 2024


		protected abstract double StrainValueOf(OsuDifficultyHitObject current);

		protected abstract double StrainValueIf(OsuDifficultyHitObject simulated, TouchHand currentHand, TouchHand lastHand);


		protected abstract double StrainValueIf(OsuDifficultyHitObject simulated, TouchHand currentHand, TouchHand lastHand);

		public abstract RawTouchSkill DeepClone();

General Touchscreen Rebalance #28396

Are you sure you want to change the base?

General Touchscreen Rebalance #28396

Conversation

BenZeng04 commented Jun 5, 2024

tsunyoku left a comment

Choose a reason for hiding this comment

tsunyoku Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rian8337 Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Givikap120 commented Jun 5, 2024

tsunyoku Jun 5, 2024 •

edited

Loading

Rian8337 Jun 5, 2024 •

edited

Loading