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

Fix cursor trail disconnect from cursor when settings or notification overlay is opened #29070

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

Cai1Hsu
Copy link
Contributor

@Cai1Hsu Cai1Hsu commented Jul 25, 2024

Description

This PR fixes an issue where the cursor trail becomes disconnected from the cursor when the settings or notification panel is opened. The problem occurs because the cursor trail's screen coordinates are not affected by the UI's horizontal shift, unlike other game objects.

To resolve this, the trail's coordinates are now stored in the CursorTrail object's local space instead of screen space. The coordinates are then converted back to screen space during drawing, ensuring they align correctly with the rest of the game objects that are affected by the panel's horizontal shift.

Screenshot

Untitled Left: Before | Right: After

- Convert cursor trail coordinates to local space before storing.
- Apply necessary transformations to align with other UI elements.
- Ensure cursor trail remains connected during UI panel movements.
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.

Looks to work fine

@peppy peppy merged commit 12f237a into ppy:master Jul 25, 2024
13 checks passed
@Cai1Hsu Cai1Hsu deleted the cursor-trail-disconnect branch July 25, 2024 13:35
@@ -285,9 +285,11 @@ protected override void Draw(IRenderer renderer)
if (time - part.Time >= 1)
continue;

Vector2 screenSpacePos = Source.ToScreenSpace(part.Position);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be done in the DrawNode.

Copy link
Member

Choose a reason for hiding this comment

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

For those that don't know why, can you provide an explanation of what is wrong and why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Simply because this is accessing a member which is on the update thread. For instance, this could cause validations which is a recursive query up the scene graph with who knows what effects - the important part is that it's a cross thread access.

The only time you should access "Source" is inside ApplyState.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, assuming you're gonna push a fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my bad implementation, I should have been aware of that

Copy link
Contributor

Choose a reason for hiding this comment

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

All good, it's why we have reviews :)

@huyenden
Copy link

@Cai1Hsu I think I have a bug here, can you check it out. #29214

@Cai1Hsu
Copy link
Contributor Author

Cai1Hsu commented Jul 31, 2024

@hachuyenden you are correct, partly because before that pr trails were not rotated because they were drawn at the positions where they were actually spawned, but after that pr, positions of the trail parts will be recalculated relative to the CursorTrail drawable when rendering.

btw I didn't see breaks that are so obvious like in your pictures, only got a little when the fps drops.

@huyenden
Copy link

huyenden commented Jul 31, 2024

@hachuyenden you are correct, partly because before that pr trails were not rotated because they were drawn at the positions where they were actually spawned, but after that pr, positions of the trail parts will be recalculated relative to the CursorTrail drawable when rendering.

Can you tweak the cursor trail a bit to look like before, but still keep the positive impact your pr does?
Let's say add a condition so that BR doesn't impact for example

btw I didn't see breaks that are so obvious like in your pictures, only got a little when the fps drops.

If you use a 12 rpm Barrel roll, the effect can be clearly seen, because a normal 0.5 rpm Barrel roll will not be detected.
I'm using OpenGL 60fps, although using other renderers gives the same results.
Guess my machine is affected more, here is another picture and still the same result with the current update
osu_2024-07-31_19-20-11

@Cai1Hsu
Copy link
Contributor Author

Cai1Hsu commented Jul 31, 2024

the CursorTrail only generates trail parts when receiving a mouse move event, this means if ur mouse is not moving or moving too slowly that doesn't generate a move event, the parts were not generated, but the drawable is constantly rotating, the end of last trail moved away, so maybe several milliseconds later when generating other parts, it looks like broken.

i recorded a video showing this.

2024-07-31.22-17-43.mp4

the reason your symptom so obvious is probably the trails were actually considered as different parts.

@huyenden
Copy link

the CursorTrail only generates trail parts when receiving a mouse move event, this means if ur mouse is not moving or moving too slowly that doesn't generate a move event, the parts were not generated, but the drawable is constantly rotating, the end of last trail moved away, so maybe several milliseconds later when generating other parts, it looks like broken.
i recorded a video showing this.
the reason your symptom so obvious is probably the trails were actually considered as different parts.

It's strange that your machine doesn't have any problems while mine does.

I'll be playing Barrel roll 12 a lot in the future so I'm very concerned about the cursors if that not look as good as before.

Anyway. Nice avatar fumo. If you can improve this PR further I'd be very interested.

@huyenden
Copy link

By the way here is my cursor trail clip

osu.2024-07-31.21-31-52-935.mp4

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