fixes for "On Display Messages" #8923
Conversation
Filoppi
changed the title
|
@dolphin-emu-bot rebuild |
|
We should check if our various start-of-emulation-warning messages still work with this. Things like, GPU doesn't support important feature or GCI failed to load come to mind. |
If you don't want to test, I can either get rid of the clean before the window open. At worse, you are going to see a few messages on screen for 1 frame, which is not a big deals, but from my tests, I don't remember any appearing. |
|
I've implemented it in the alternative way that I had already described. The explanation is above. |
| if (time_left <= 0) | ||
| // Make sure we draw them at least once if they were printed with 0ms, | ||
| // unless enough time has expired, in that case, we drop them | ||
| if (time_left <= 0 && (msg.ever_drawn || -time_left >= MESSAGE_DROP_TIME)) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
That would mean that all the expired messages draw one time more than necessary (which also mean they wouldn't follow the length of time you had specified). I think it's better to have a variable non const.
There was a problem hiding this comment.
Hypothetical case but that would break when going over ~1000fps though (time accuracy is 1ms):
Add a message: timestamp 53 (53ms current time + 0 duration).
Draw messages at time 53ms.
Add another message which is exactly the same: timestamp 53 (if we are still within the same ms).
Draw messages at time 53ms again: the previous message nor the new message will draw now.
There was a problem hiding this comment.
That's true. Sorry, my initial comment was not well thought out. However, still not a fan of that variable so I have a different suggestion.
If I'm understanding things correctly, I think what you're trying to account for are messages that get missed because they expire sometime before DrawMesages() gets called (or in the case of a start/stop of the software expire sometime in between DrawMessages() calls).
I think a solution to that problem is to add another timestamp.
u32 s_last_drawn_timestamp = 0;
void DrawMessages()
{
if (time_left <= 0 && (s_last_drawn_timestamp > msg.timestamp || -time_left >= MESSAGE_DROP_TIME))
...
s_last_drawn_timestamp = now;
}
There was a problem hiding this comment.
Yeah I was thinking about changing >= to >. Let me finish reasoning :).
If edits are requested for such small things none of my other changes will ever go it, not within 3+ years
There was a problem hiding this comment.
changing >= to > has the opposite effect of the problem I mentioned above. If going to extremely high fps, they could now draw more than once. Which is not really a problem, but is theoretically wrong, and given that with the ever_drawn flag it works perfectly, I don't think it would be worth changing.
There was a problem hiding this comment.
If edits are requested for such small things none of my other changes will ever go it, not within 3+ years
It's just an unfortunate consequence of this being an open-source repo with very few developers doing constant reviews (and even fewer with merge powers). Because of that, code changes can take a while. Only thing that gets merged fast are code-cleanup changes and even that can sometimes take a while depending on the size.
If going to extremely high fps, they could now draw more than once.
I personally think that this is a fine trade-off for removing that variable (that is included on every message) and returning const correctness. When you get to that high framerate, timing accuracy isn't going to matter that much. If we really want to fix this, we need a more granular clock. I'm going to guess we aren't using std::chrono in Timer because it didn't exist back then.
EDIT: actually we could use GetTimeUs() for more granularity. But really we should enhance Timer to return chrono times, that way comparisons will be a lot cleaner.
There was a problem hiding this comment.
Well on screen messages are not logs, there are barely any, what's the problem with adding a bool? And it's not like it's not const correct, ever_drawn is changed by the Draw() function. That makes sense.
Changing the timer won't exactly fix the problem, depending on the granularity. But that's kind of beyond the scope of this PR, wanted to avoid changing even more.
There was a problem hiding this comment.
I'm definitely nitpicking. Sorry to take up so much of your time. It just felt counterintuitive for DrawMessage to be changing Message even though I see your reason for doing so. My hope was to come up with a solution that would allow for the Message to be const again.
Anyway, I don't have merge powers, so someone else will need to review this regardless. My apologies again. These are good changes overall.
-They might have never drawn if DrawMessages wasn't called before they actually expired -Their fade was wrong if the duration of the message was less than the fade time This makes them much more useful for debugging, I know there might be other means of debugging like logs and imgui, but this was the simplest so that's what I used. If you want to print the same message every frame, but with a slightly different value to see the changes, it now work. To compensate for the fact that they are now always rendered once, so on start up a lot of old messages (printed while the emulation was off) could show up, I've added a "drop" time, which means if a msg isn't rendered for the first time within that time, it will be dropped and never rendered.
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
|
This has been sitting for a while and it seems like everyone here is in agreement that this is an improvement. I've gone through and tested it and on-screen display messages at startup are fine. I tested purposefully duplicate GCI files for something that would cause issues on boot. |
Mainly fixed 2 things on "On Display Messages":
-They might have never drawn if DrawMessages wasn't called before they actually expired.
-Their fade was wrong if the duration of the message was less than the fade time.
This makes them much more useful for debugging, I know there might be other means of debugging like logs and imgui, but this was the simplest so that's what I used.
If you want to print the same message every frame, but with a slightly different value to see the changes, it now work.
To compensate for the fact that they are now always rendered once, so on start up a lot of old messages (printed while the emulation was off) could show up, I've added a "drop" time, which means if a msg isn't rendered for the first time within that time, it will be dropped and never rendered.
A previous solution was to clean all the messages before the emulation thread started, but that might have had side effects.