IndexGenerator: Fix off-by-one in GetRemainingIndices #11402
Conversation
| // subtract 1 from max_index to correctly reserve one index for primitive restart. | ||
| // | ||
| // Pocoyo Racing uses a draw command with 0xffff vertices, which previously caused issues; see | ||
| // https://bugs.dolphin-emu.org/issues/13136 for details. |
There was a problem hiding this comment.
Do we have any fifologs that test the primitive restart behavior?
There was a problem hiding this comment.
Yes, both Mario Tennis fifologs; see #10414.
There was a problem hiding this comment.
Oh, though I guess I should clarify: the primitive restart referred to here is different from the one used on the GameCube/Wii. Here, primitive restart is used so that we can do a single draw call with a long triangle strip, even when the input is composed of multiple triangle strips (or even individual triangles). On the gamecube/wii, at least for mario tennis, it seems more like something where some vertices contain invalid data and need to be skipped (and there's less of a cost for doing multiple draw calls, I think?). On most platforms, dolphin uses primitive restart for everything, even converting lists of individual triangles into triangle strips composed of 3 vertices and then a primitive restart.
There was a problem hiding this comment.
Ok, just wanted to make sure the removal of the -1 was tested. I remember the primitive restart functionality, Stenzek and I had to debug my card...it turned out Vulkan+primitive restart didn't work for it :D
There was a problem hiding this comment.
Yeah, that -1 functionality still exists (it's handled in the vertex loaders, not in IndexGenerator). IndexGenerator also still has its -1 (s_primitive_restart).
|
Fixes the issue and the changes make sense to me |
| return max_index - m_base_index - 1; | ||
| if (!(m_base_index <= max_index)) [[unlikely]] | ||
| { | ||
| ASSERT_MSG(VIDEO, m_base_index <= max_index, "GetRemainingIndices would overflow: {} <= {}", |
There was a problem hiding this comment.
Should this be one of those game quirks (I think it is a stat?)
There was a problem hiding this comment.
I don't think this would be a useful game quirk; this one theoretically should never happen (it means we've already written too many indices). The ones in VertexManagerBase could possibly be a game quirk but I don't know how useful it would be.
There was a problem hiding this comment.
Ah ok. I was kinda going off of the comment in the OP:
There is still a situation where this could come up: if over 0x3fff vertices are used while drawing points or lines, and vertex shader point/line expansion is enabled, we will run out of indices. I don't know of any case where that happens in practice, though.
There was a problem hiding this comment.
On further thought, I'm going to convert the ones in VertexManagerBase into assertions instead of just logs. I think a game quirk is overkill, but it's a situation that should be obvious to the user.
Fixes incorrect logspam when the buffer needed to be reset on flushes (which we already were doing, but 52feed0 moved it to after the check was made). This is https://bugs.dolphin-emu.org/issues/10312. I also converted it to an assert, as if this does happen, things are going to render incorrectly, so we want to make it obvious.
Pokechu22
force-pushed
the
too-many-indices
branch
from
fe2896b
to
29046c1
Compare
|
|
||
| return max_index - m_base_index - 1; | ||
| if (!(m_base_index <= max_index)) [[unlikely]] |
There was a problem hiding this comment.
Seems like this can be inverted a little so one doesn't need to mentally do it themselves when reading
| if (!(m_base_index <= max_index)) [[unlikely]] | |
| if (m_base_index > max_index) [[unlikely]] |
There was a problem hiding this comment.
This was deliberate to match the condition in the assert, but it looks like compilers (at least MSVC) don't merge the two together, so there isn't a significant difference.
I prefer the <= version because it makes it more obvious that it would overflow, though. I could put a separate variable along the lines of const bool subtraction_would_overflow = (m_base_index <= max_index) and use it in both places, maybe?
This assertion is currently triggered by Pocoyo Racing (https://bugs.dolphin-emu.org/issues/13136).
Pokechu22
force-pushed
the
too-many-indices
branch
from
29046c1
to
cefcd9c
Compare
Fixes Pocoyo Racing (issue 13136), and also log spam about "Too little remaining index values. Use 32-bit or reset them on flush." on Super Mario Sunshine's grass special stage and some smash custom levels (issue 10312).
The problem with Pocoyo Racing is that it uses 65535 (0xffff) vertices in a single draw command, which caused
GetRemainingIndicesto overflow, acting as if there was space for more vertices even though there wasn't. Then the game draws 3681 more vertices, and the indices corresponding to those vertices are all messed up because of the overflow, resulting in vertex explosions. The index generator actually can handle 0xffff indices for vertices with one more value reserved for primitive restart, so the fix is just to let that work withGetRemainingIndices.The logspam is misleading, as we already reset the index generator on flushes at the time that message was added (470c9ff); it's just that in 52feed0 the resetting was moved to after that log message.
There is still a situation where this could come up: if over 0x3fff vertices are used while drawing points or lines, and vertex shader point/line expansion is enabled, we will run out of indices. I don't know of any case where that happens in practice, though.
Pocoyo Racing still has some very weird behavior with fifologs, which is not addressed by this PR.