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 a replay analysis overlay #27334

Merged
merged 46 commits into from
Sep 5, 2024
Merged

Conversation

Sheppsu
Copy link
Contributor

@Sheppsu Sheppsu commented Feb 23, 2024

Design is based upon Rewind and discussion #26856.

Adds:

  • An analysis settings box right under playback when viewing a replay. Only shows for game modes that have one implemented, which is just osu!std in this PR.
  • Toggle for hit markers that show where the player clicked and whether it was a left or right click.
  • Toggle for aim markers that show where the cursor is on each frame.
  • Toggle for an aim line that shows the cursor's path.
  • Test for OsuAnalysisContainer

Video showing it with different toggles applied: https://media.sheppsu.me/view?n=gPqsCeBmyrw (video was too big to attach :P )

Sheppsu and others added 8 commits February 1, 2024 10:19
committing early version for others in the discussion to do their own testing with it
Hit & aim markers are skinnable. Hidden can be toggled off. Aim line with skinnable color was added. The fadeout time is based on the approach rate. Cursor hide fixed.
@Sheppsu Sheppsu marked this pull request as ready for review February 23, 2024 03:01
@peppy peppy self-requested a review February 23, 2024 05:17
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.

not reall doing a full review just obvious issues

osu.Game.Rulesets.Osu/Skinning/Default/DefaultHitMarker.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/UI/HitMarkerContainer.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/UI/HitMarkerContainer.cs Outdated Show resolved Hide resolved
Uses pooling for all analysis objects and creates the lifetime entries from replay data when the analysis container is constructed.
improve default design
better to add skinnable elements later
@Sheppsu
Copy link
Contributor Author

Sheppsu commented Feb 28, 2024

Just going to boil it down to the main components, so skinning can be added later.

@AadilRn
Copy link

AadilRn commented Mar 5, 2024

You should probably break this PR up into different components instead of just having 1 big PR that is essentially adding "Rewind" to replays.

@Sheppsu
Copy link
Contributor Author

Sheppsu commented Mar 5, 2024

I think the only part where it would make sense to split up is between the base analysis settings implementation and the osu!std analysis implementation, but I don't know that it's really necessary to do that.

@tybug
Copy link
Member

tybug commented Sep 1, 2024

maybe I'm biased from circleguard but I think crosses look better than plusses for hit markers, going off the linked video. afaik rewind does crosses as well

@Sheppsu
Copy link
Contributor Author

Sheppsu commented Sep 1, 2024

I initially did a cross design to match rewind, but I think the difference in using a plus is negligible. As peppy mentioned in his review, it could be confusing to use a cross since that design represents a miss.

@peppy peppy self-requested a review September 2, 2024 03:07
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.

Another very general pass..

osu.Game.Rulesets.Osu.Tests/TestSceneHitMarker.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Play/PlayerSettings/AnalysisSettings.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/UI/OsuAnalysisSettings.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/UI/OsuAnalysisContainer.cs Outdated Show resolved Hide resolved
@vatei
Copy link

vatei commented Sep 4, 2024

I've done an initial visual pass, open to feedback.

Not a super big fan of the alternating colors either, it feels like they should match the corresponding judgement rather than being entirely separate.

Disagree, it's useful to see the different keys for identifying issues such as a weaker finger

@tybug
Copy link
Member

tybug commented Sep 4, 2024

(haven't built myself, going off @dani211e's video (thanks!)) -

  • circle markers are a big improvement over the plusses or even crosses
  • the cursor trail seems quite long. I made this configurable in circleguard and by default it was ~3/5 that length. It should probably roughly match the fadeout of the hitobjects? I don't expect this to be configurable off the bat, though it may be nice.
  • I'm personally not a big fan of coloring the markers based off the keypress, only because there's no intuitive mapping f: color |-> keypress. What is blue? what is yellow? do they even correspond to keypresses? I'd figure it out after a while, but corresponding to judgments is much more intuitive. I know we dislike toggles over here, but maybe this one really should be a toggle, defaulting to judgment. fwiw I never colored the hit markers in circleguard, though I got quite a few requests for it, so it's clearly something people want.

@peppy
Copy link
Member

peppy commented Sep 5, 2024

@Sheppsu one other issue i'm not going to fix in this PR is that rewinding draws previous frames in front of newer ones rather than behind. maybe something you can add to the todo list.

@Sheppsu
Copy link
Contributor Author

Sheppsu commented Sep 5, 2024

@Sheppsu one other issue i'm not going to fix in this PR is that rewinding draws previous frames in front of newer ones rather than behind. maybe something you can add to the todo list.

Ah ok, I'll look into that for a follow-up.

@peppy
Copy link
Member

peppy commented Sep 5, 2024

I'm going to lock in these visuals for now after one too many passes:

osu! 2024-09-05 at 06 06 53

osu! 2024-09-05 at 06 07 54 osu! 2024-09-05 at 06 08 07
osu! 2024-09-05 at 06 08 17 osu! 2024-09-05 at 06 08 27
osu! 2024-09-05 at 06 09 02 osu! 2024-09-05 at 06 09 08
osu! 2024-09-05 at 06 09 14 osu! 2024-09-05 at 06 09 21

@peppy peppy force-pushed the replay-analysis-settings branch from c77f1dc to 167e3a3 Compare September 5, 2024 07:25
@bdach bdach self-requested a review September 5, 2024 07:48
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.

Seems fine, but @peppy please double-check my fix at b9ddac4 (having the alpha set inside requireDisplay would cause the visibility toggles to not toggle off properly if they were the only ones enabled at the time of toggle off - this was luckily caught by tests)

@peppy
Copy link
Member

peppy commented Sep 5, 2024

Fix seems fine, thanks.

@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 Sep 5, 2024
@peppy peppy merged commit 446c810 into ppy:master Sep 5, 2024
8 of 9 checks passed
@Sheppsu Sheppsu deleted the replay-analysis-settings branch September 5, 2024 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:replay next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! ruleset/osu! size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants