VideoCommon: Fix scissorOffset, handle negative value correctly #9660
Conversation
|
This PR fix a bug about BPMemory.scissorOffset. This RP fix the bug: Super Mario Galaxy 1/2 - Roar shockwave does not display, reported here: Issues #8327. And should fix any other similar bug. Before fix:
After fix:
Reference: |
|
I'm not familiar with git, I try to Squash Commits, but just broke the PR. |
You should be able to just click the reopen button at the bottom (in the same place as the close button was). |
|
The reopen button is dimmed, I guess I don't have permission to reopen it. |
| /* NOTE: the minimum value here for the scissor rect and offset is -342. | ||
| * GX internally adds on an offset of 342 to both the offset and scissor | ||
| * coords to ensure that the register was always unsigned. | ||
| /* NOTE: the minimum value here for the scissor rect is -342. | ||
| * GX internally adds on an offset of 342 to scissor coords to | ||
| * ensure that the register was always unsigned. | ||
| * | ||
| * The code that was here before tried to "undo" this offset, but | ||
| * since we always take the difference, the +342 added to both | ||
| * sides cancels out. */ | ||
|
|
||
| /* The scissor offset is always even, so to save space, the scissor offset | ||
| /* NOTE: With positive scissor offset the scissor rectangle is shifted left and/or up; | ||
| * and with negative scissor offset, the scissor rectangle is shifted right and/or down. | ||
| * | ||
| * GX internally also adds on an offset of 342 to scissor offset. | ||
| * The scissor offset is always even, so to save space, the scissor offset | ||
| * register is scaled down by 2. So, if somebody calls | ||
| * GX_SetScissorBoxOffset(20, 20); the registers will be set to 10, 10. */ | ||
| const int xoff = bpmem.scissorOffset.x * 2; | ||
| const int yoff = bpmem.scissorOffset.y * 2; | ||
| * GX_SetScissorBoxOffset(20, 20); the registers will be set to (10 + 171, 10 + 171). | ||
| * | ||
| * The scissor offset register is 10bit signed [-512, 511]. | ||
| * (e.g. In Super Mario Galaxy 1 and 2, during the Boss roar effect, | ||
| * the scissor offset register will be set to -61, so the scissor offset is -464) | ||
| */ |
There was a problem hiding this comment.
Any suggestion about this comments is welcome, so we can make it more clear.
There was a problem hiding this comment.
"With positive scissor offset" should be "With a positive scissor offset" (and there should be a comma after offset on both lines).
It might be worth clarifying that "GX internally also adds on an offset of 342 to scissor offset." means "The GX SDK functions internally add an offset of 342".
It would also be useful to elaborate on the math, e.g. "the scissor offset register will be set to -61, so the scissor offset is (-61)*2 - 342 = -122 - 342 = -464", or maybe it makes more sense in the opposite direction: "for a scissor offset of -464, the scissor offset register will be set to (-464 + 342)/2 = -122/2 = -61".
|
There's no need to reopen the last one since this one is opened now. Just as a FYI for the future it is better to make a new branch based on master and make commits there rather than making them directly on master in your fork. |
VideoCommon: Change the type of BPMemory.scissorOffset to 10bit signed: S32X10Y10 VideoBackends: Fix Software Clipper.PerspectiveDivide function, use BPMemory.scissorOffset instead of hard code 342
|
|
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
|
Just noticed it was squashed. |
|
Final FifoCI results: https://fifo.ci/version/18e84361d925af09586b940b265f10e501e889d6/ There were 9 changes on |
VideoCommon: Fix scissorOffset, handle negative value correctly
VideoBackends: Fix Software Clipper.PerspectiveDivide function, use bpmem.scissorOffset instead of hard code 342
Origin pull request: #9656
Can some one help me to reopen #9656, I found a way to reset the commits.