Fifo analyzer quality of life improvements #9540
Conversation
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
3 times, most recently
from
bf8b16a
to
72a6da5
Compare
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
3 times, most recently
from
cfcfb78
to
cb3c5dc
Compare
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
from
cb3c5dc
to
aabb2f1
Compare
|
Here's what the new descriptions for indexed XF loads and for primitive operations look like: Super Mario Sunshine
Super Monkey Ball
You still need to look at other commands to figure out exactly what the different parts of the primitive commands mean, but the spaces should still make it a lot easier. |
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
2 times, most recently
from
e8011cb
to
ac97c8c
Compare
|
Person who merges this, this fixes: https://bugs.dolphin-emu.org/issues/12452 ! |
|
@Pokechu22 - I'm assuming this is ready to go? |
|
Yes, it is ready. |
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
from
ac97c8c
to
7f4968b
Compare
| @@ -205,98 +205,16 @@ void VertexLoader::CompileVertexTranslator() | |||
| m_native_vtx_decl.colors[i].components = 4; | |||
| m_native_vtx_decl.colors[i].type = VAR_UNSIGNED_BYTE; | |||
| m_native_vtx_decl.colors[i].integer = false; | |||
| switch (m_VtxDesc.low.Color[i]) | |||
There was a problem hiding this comment.
It'd be a pain but seeing as this class already has some unit-tests, I wonder if it'd be beneficial to write some tests before removing this with the improved replacement.
(though from what I can tell, everything is good and this is a vast improvement)
There was a problem hiding this comment.
I don't feel like writing tests exhaustive enough to cover every combination of color settings; although they'd be useful to have, it seems like a fair bit of effort which I don't want to include in this PR. The refactoring I did here is basically just converting the switch statement into two arrays, without changing the functions themselves, so I don't think there's too much I could have broken.
It's also worth noting that the regular VertexLoader is not actually used by anything currently (only VertexLoaderARM64 and VertexLoaderX64 are used in practice since Dolphin only supports X64 and ARM64); it can be used by editing VertexLoaderBase::CreateVertexLoader (and in particular by defining COMPARE_VERTEXLOADERS; I've run the existing unit tests with that enabled and they all pass and some non-exhaustive gameplay testing also showed no issues).
|
|
||
| // GC vertex format | ||
| TVtxAttr m_VtxAttr; // VAT decoded into easy format | ||
| VAT m_VtxAttr; |
There was a problem hiding this comment.
On a related note, I'm not sure if the comment below really makes sense anymore:
dolphin/Source/Core/VideoCommon/VertexLoaderBase.h
Lines 90 to 91 in c350da0
I don't think there's any way to avoid use of m_VtxDesc; the comment only makes sense if the goal was to also include that info in TVtxAttr, in which case the comment doesn't apply anymore.
There was a problem hiding this comment.
Comment removed (and also, m_VtxAttr and m_VtxDesc are now changed to const in "Eliminate TVtxAttr" instead of "Move vertex size and component calculation to VertexLoaderBase").
| // It's not really useful to have a massive unreadable hex string for the object primitives. | ||
| // Put it in the description instead. | ||
|
|
||
| // new_label += QStringLiteral(" "); |
There was a problem hiding this comment.
Still a todo or should this be removed?
There was a problem hiding this comment.
More of a leftover/sample implementation in case it's something that is wanted for the future. Perhaps #if 0 would be better than commenting it out?
There was a problem hiding this comment.
I've replaced it with a #ifdef (and also adjusted the implementation, since it wasn't actually correct for multiple objects).
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
from
7f4968b
to
106bb3e
Compare
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
from
106bb3e
to
c350da0
Compare
It is no longer relevant for the current set of loaders after 7030542. If it becomes relevant again, a static function named IsUsable or IsCompatibleWithCurrentMachine or something would be a better approach.
Now that this is only called when playback actually starts (and not on unpausing), this change makes the experience a bit better (no more missing objects from not having reset the from object after changing FIFOs).
A single object can be selected instead of 2 (it was already inclusive internally), and the maximum value is the highest number of objects in any frame (minus 1) to reduce jank when multiple frames are being played back.
The 'zero frames in the range' check can be removed because now there is always at least 1 frame; of course that might be the same frame over and over again, but that's still useful for e.g. Free Look (and the 1 frame repeating effect already occurred when frame count was exclusive).
If the number of objects varied, this would result in either missing objects on some frames, or too many objects on some frames; the latter case could cause crashes. Since it used the current frame to get the count, if the FIFO is started before the FIFO analyzer is opened, then the current frame is effectively random, making it hard to reproduce consistently. This issue has existed since the FIFO analyzer was implemented for Qt.
…tion It still tries to update the description on clearing, potentially with bad data.
This way, it can be focused with the render window behind it, instead of having the main window show up and cover the render window. This is useful for adjusting the object range, among other things.
This also shows the register updates for object 0, which were previously not visible(!)
- Only one search result is generated per command/line, even if there are multiple matches in that line. - Pressing enter on the edit field begins a search, just like clicking the begin button. - The next and previous buttons are disabled until a search is begun. - The search results are cleared when changing objects or frames. - The previous button once again works (a regression from the previous commit), and the register updates and graphics data for the correct object are searched. - currentRow() never returns -1, so checking that is unnecessary (and misleading). - The 'Invalid search parameters (no object selected)' previously never showed up before because FRAME_ROLE is present if and only if OBJECT_ROLE is present.
Since the description updating is tied to the selection changing on the detail list, and the detail list is recreated on each object change, behavior was somewhat broken. Clearing the list changed the current row to zero, but nothing else (particularly m_object_data_offsets) had been updated, so the description was not necessarily correct (this is easier to observe now since the vertex data is at the end, so it's easier to get different lengths of register updates). Furthermore, subsequent clears did not update the current row since there was no visible selection, so it only changed the description once. The current row is now always set to zero, which forces an update (and also scrolls the list back to the top). The presence of FRAME_ROLE and OBJECT_ROLE are also checked so that the description is cleared if no object is selected.
Pokechu22
force-pushed
the
better-fifo-analyzer-part-2
branch
from
90c7fde
to
77b1cca
Compare
| // Note that frame_info.objectStarts[object_nr] is the start of the primitive data, | ||
| // but we want to start with the register updates which happen before that. | ||
| const u32 object_start = (object_nr == 0 ? 0 : frame_info.objectEnds[object_nr - 1]); | ||
| const u32 object_size = frame_info.objectEnds[object_nr] - object_start; |
There was a problem hiding this comment.
Ok... but now you are missing the register updates after the final object in the frame.
Also, the terminology is getting mixed up here. In the fifo playback code, Objects are explictly primitive draws and just the primitive draws. They are only defined so that the object skipping code can skip the objects and not the register writes around it.
Register writes being connected to objects is something that only happens in this UI code
Actually a pretty good way to test it. If it's broken you, dolphin will crash or panicalert. |
Part 2 of #9497.
Currently, the first commit stubs the changes it makes that are depended upon by later commits; once that is eventually emerged it should cleanly rebase with that commit dropped.