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

Achievements: Fix leaderboard timers persisting #12352

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

dreamsyntax
Copy link
Contributor

@dreamsyntax dreamsyntax commented Feb 24, 2025

Description of Changes

Remove setting indicator active state in HandleLeaderboardTrackerUpdateEvent per #12352 (comment)

Image showing the issue:
image

Demo video showing the issue and before/after:
https://github.com/user-attachments/assets/f04ed5ce-5137-42e8-96be-f284766cdc0a

Rationale behind Changes

Fix the timer being set if events come in out of order.

OLD PROPOSAL (outdated)

Description of Changes

Drop INDICATOR_FADE_IN_TIME (0.1) / INDICATOR_FADE_OUT_TIME (0.5)
Replace with INDICATOR_FADE_TIME with value 0.1.
This makes it highly unlikely to experience a persistent timer.

Rationale behind Changes

Depending on the rate that new timer/achievement (bottom right) indicators are added, it is possible to have indicators that never disappear until you end the emulation.
The new default of 0.1 does not look jarring, and avoids the issue.

Suggested Testing Steps

Use any version of PCSX2 that supports the Achievement UI feature. I used v2.3.171.

  1. Load any game where you can rapidly start a leaderboard attempt that involves a timer. My demo video is using Spyro: A Hero's Tail.
  2. Rapidly invoke leaderboard attempt end/starts. Best tested on high end hardware that can run a title at a fast rate; Recommend fast forward of 400% for testing to assist with invoking fast starts.
  3. See demo video above of an example of before and after.

@F0bes
Copy link
Member

F0bes commented Feb 24, 2025

Would you be able to provide a memory card and or save state so I can try this locally? Specifically the Spyro challenge in your video. It's not immediately apparent why this happens when I look at the code.

@dreamsyntax
Copy link
Contributor Author

@F0bes sample:

Load SLOT1 in game, will be placed immediately in front of the challenge minigame.
SLUS-20884-memcard-pr-12352.zip

@F0bes
Copy link
Member

F0bes commented Feb 24, 2025

So it seems to be some sort of race condition.
We get a hide event, we set the timer to inactive, but then we get an update event from RA (who knows why it's out of order), and we set the timer to active again.

I think it's wrong to assume an updated event should be forced active, especially if we were just told to hide it. If it's supposed to be persistent like that, then we would have an issue with the current implementation after inactive timers are deleted.

Going to Achievements::HandleLeaderboardTrackerUpdateEvent and removing the it->active = true; at the bottom of the function fixes the stuck indicators. Feel free to amend your PR to include the change. I'll leave it up to you to decide if you want to keep the fade time changes.

Removes setting the leaderboard timer to active on receiving an update
event. This fixes having multiple timers stuck on the screen.
@dreamsyntax dreamsyntax changed the title Achievements: Make indicator fade always 0.1s Achievements: Fix leaderboard timers persisting Feb 24, 2025
@dreamsyntax
Copy link
Contributor Author

Feel free to amend your PR to include the change. I'll leave it up to you to decide if you want to keep the fade time changes.

Since this is the root cause, I've updated the commit and PR description. Reverted the fade changes since they are merely preference.

Thank you for tracking that down.

@F0bes
Copy link
Member

F0bes commented Feb 24, 2025

Feel free to amend your PR to include the change. I'll leave it up to you to decide if you want to keep the fade time changes.

Since this is the root cause, I've updated the commit and PR description. Reverted the fade changes since they are merely preference.

Thank you for tracking that down.

Just to make sure, you've tested this and can confirm it fixes the issue for you?

@dreamsyntax
Copy link
Contributor Author

Just to make sure, you've tested this and can confirm it fixes the issue for you?

Yes. There is a few frames where the old timer persists, but that is expected until the fade completes.

Copy link
Member

@F0bes F0bes left a comment

Choose a reason for hiding this comment

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

Thanks :)

@F0bes F0bes merged commit 20b1190 into PCSX2:master Feb 24, 2025
12 checks passed
@dreamsyntax dreamsyntax deleted the indicator-time branch February 24, 2025 19:26
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.

2 participants