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

Make judgements follow hitcircles and enable them in magnetised, repel and depth #27977

Merged
merged 11 commits into from
May 30, 2024

Conversation

DavidBeh
Copy link
Contributor

Addresses #20139

Makes judgements appear based on the DrawableHitobjects position instead of the Hitobjects position which is not updated by mods moving single Hitobjects. They move with hitobjects as long as the drawables are alive, so they move with the shadow of hits and stay on misses.

Judgements are enabled for magnetised, repel and depth.

@DavidBeh DavidBeh changed the title Magnetised judgements Make judgements follow DrawableHitObjects and enable them in magnetised, repel and depth Apr 23, 2024
@bdach bdach changed the title Make judgements follow DrawableHitObjects and enable them in magnetised, repel and depth Make judgements follow hitcircles and enable them in magnetised, repel and depth May 29, 2024
@bdach
Copy link
Collaborator

bdach commented May 29, 2024

Measured overhead in profiling seems acceptable:

1716974984

diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
index 98fb99609a..475c2bac72 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
@@ -42,6 +42,11 @@ protected override void Update()
         {
             base.Update();
 
+            theThingThatIWishToProfileRightNow();
+        }
+
+        private void theThingThatIWishToProfileRightNow()
+        {
             if (JudgedObject is DrawableOsuHitObject osuObject && JudgedObject.IsInUse)
             {
                 Position = osuObject.ToSpaceOfOtherDrawable(osuObject.OriginPosition, Parent!);

bdach
bdach previously approved these changes May 29, 2024
@bdach bdach requested a review from a team May 29, 2024 10:03
@peppy peppy self-requested a review May 29, 2024 14:54
@peppy
Copy link
Member

peppy commented May 29, 2024

I'm fine with this on a code level, but depth mod may need some adjustments to ensure the judgements keep moving, if that is the goal:

osu.2024-05-29.at.14.56.34.mp4

A potentially more appropriate path would be to lock the position when judgement happens (based on the DrawableHitObject still, to still fix the main reasons as to why they were disabled), which may feel better.

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Might need a bit of thought in terms of game feel

@peppy
Copy link
Member

peppy commented May 29, 2024

Proposed:

osu.2024-05-29.at.15.06.53.mp4
diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
index 98fb99609a..6e252a53e2 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
@@ -17,6 +17,8 @@ public partial class DrawableOsuJudgement : DrawableJudgement
         [Resolved]
         private OsuConfigManager config { get; set; } = null!;
 
+        private bool positionTransferred;
+
         [BackgroundDependencyLoader]
         private void load()
         {
@@ -36,16 +38,20 @@ protected override void PrepareForUse()
 
             Lighting.ResetAnimation();
             Lighting.SetColourFrom(JudgedObject, Result);
+
+            positionTransferred = false;
         }
 
         protected override void Update()
         {
             base.Update();
 
-            if (JudgedObject is DrawableOsuHitObject osuObject && JudgedObject.IsInUse)
+            if (!positionTransferred && JudgedObject is DrawableOsuHitObject osuObject && JudgedObject.IsInUse)
             {
                 Position = osuObject.ToSpaceOfOtherDrawable(osuObject.OriginPosition, Parent!);
                 Scale = new Vector2(osuObject.HitObject.Scale);
+
+                positionTransferred = true;
             }
         }
 

(there may be a better way to write this)

@DavidBeh
Copy link
Contributor Author

I am not sure if that looks good with Argon Skins and hit lighting because these also move after the result

@peppy peppy self-requested a review May 30, 2024 03:57
@peppy
Copy link
Member

peppy commented May 30, 2024

Argon for reference:

osu.2024-05-30.at.03.59.16.mp4

I think it looks fine? Also consider that if anything, I'd say we need to move the hit lighting out of the judgement in argon at some point 😅

Looks less bad with mods like depth active.

Co-authored-by: Dean Herbert <pe@ppy.sh>
@bdach
Copy link
Collaborator

bdach commented May 30, 2024

I agree that the proposal above looks less bad than the original and the patch seems fine so I've applied.

@smoogipoo smoogipoo merged commit 3c2599c into ppy:master May 30, 2024
9 of 17 checks passed
bdach added a commit to bdach/osu that referenced this pull request Jun 25, 2024
Regressed in ppy#27977.

Bit ad-hoc but don't see how to fix without just reverting the change.
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.

4 participants