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

Trophy notification improvements #10482

Merged
merged 4 commits into from Jun 25, 2021
Merged

Trophy notification improvements #10482

merged 4 commits into from Jun 25, 2021

Conversation

Kilowog01
Copy link
Contributor

Same trophy notification, but better. The notification is no longer squished in the corner and readability has been improved a lot.
Size, color, transparency, spacing, position and duration are now the same as the original PS3 trophy notification.

better

What is still missing:

  • Icon indicating the type of trophy
  • Sound
  • Rounded edges
  • Fade-in and fade-out animation
  • Font

Comparison:
After / Original
Before

Rpcs3.Repositories.2021.06.21.-.07.02.57.37.mp4

rpcs3/Emu/RSX/Overlays/overlay_trophy_notification.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/Overlays/overlay_trophy_notification.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/Overlays/overlay_trophy_notification.cpp Outdated Show resolved Hide resolved
{
if (!sliding_animation.active)
{
sliding_animation.end = { -f32(frame.w), 0, 0 };
sliding_animation.end = { -f32(frame.w*1.25), 0, 0 };
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose you want to speed up the leaving animation?

Anyway. it's missing some spaces

Suggested change
sliding_animation.end = { -f32(frame.w*1.25), 0, 0 };
sliding_animation.end = { -f32(frame.w * 1.25f), 0, 0 };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the notification not fully sliding back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the animation is broken because you moved the notification position to the right.
It's probably better to use that instead of some arbitrary factor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sent a commit without this part. Also fixed the formatting. It's ready to merge.

Comment on lines 139 to 140
u16 margin_sz = 9;
frame.w = 72 + text_view.w + margin_sz;
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these values suddenly hardcoded?
the logic behind it should not change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this logic generates a giant right margin. I could make new logic if padding worked as it should, but it doesn't.
The best i can do is leave the first line like this and replace the second line with:
frame.w = margin_sz * 3 + image.w + text_view.w;

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't make sense. It never created a giant right margin before, so why should it now?

@Kilowog01 Kilowog01 marked this pull request as draft June 22, 2021 01:21
@Megamouse
Copy link
Contributor

Please don't change the other lines of code that you didn't really touch. Somehow your last commit added a lot of format changes.

@@ -133,11 +135,11 @@ namespace rsx
text_view.auto_resize();

// Resize background to cover the text
u16 margin_sz = text_view.x - image.w - image.x;
frame.w = text_view.x + text_view.w + margin_sz;
u16 margin_sz = 9;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using this margin is fine. But it should probably be used to calculate the image position etc as well for consistency

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Removal of auto resize means lots of testing specifically with different languages that dont use latin fonts. This will be needed with and without fw and also on linux and BSD. This hardcoded stuff to me just seems to be whats appealing in specific conditions. We had ugly overflowing text when using some system languages before.

Copy link
Contributor Author

@Kilowog01 Kilowog01 Jun 22, 2021

Choose a reason for hiding this comment

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

Tested Portuguese, English US, Japanese, Traditional Chinese, Korean, Russian and Danish. 18 different trophies each test. The only problem I noticed is that the first line remains in English, but this already happens in the current build so it isn't my fault. I can't test on linux and BSD.

@Kilowog01 Kilowog01 marked this pull request as ready for review June 22, 2021 19:20
@Nekotekina Nekotekina merged commit d59707b into RPCS3:master Jun 25, 2021
VelocityRa pushed a commit to VelocityRa/rpcs3 that referenced this pull request Jul 6, 2021
* Makes the text more similar to the original PS3 trophy notification.
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.

None yet

4 participants