Last active
September 20, 2025 06:18
-
-
Save Pokechu22/e9fa9037fe21312ff32475638b78ba27 to your computer and use it in GitHub Desktop.
Revisions
-
Pokechu22 revised this gist
Feb 8, 2022 . 1 changed file with 1 addition and 1 deletion.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -180,4 +180,4 @@ index 0fc4ca6785..b0b3940398 100644 return; ``` This also fixes the issue, but it's inaccurate for general emulation. However, the software renderer currently works that way due to an implementation mistake (it applied the scissor to 2 by 2 blocks of pixels, rather than to individual pixels). My [scissor pull request](https://github.com/dolphin-emu/dolphin/pull/10251) fixes that. When that issue with the software renderer is fixed, it too will produce the black pixels (unless the patch is applied). Note that there are games where this are needed (James Bond - Everything or Nothing can have shadows break, and Samurai Warriors 3 uses it to produce a vignette effect, for instance). -
Pokechu22 revised this gist
Dec 21, 2021 . 1 changed file with 1 addition and 1 deletion.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -4,7 +4,7 @@ This document looks at both the psycho cut fifolog and a rain fifolog, both of w ## The problem Specifically, the rightmost column and bottommost row of pixels are all black due to a poorly-configured scissor rectangle. The game has the scissor top-left at (342, 342), bottom-right at (980, 820), scissor offset of (342, 342), and the viewport covers (342, 342) to (982, 822). Without going into details on the calculation, this means that the viewport covers pixels from (0, 0) to (639, 479) inclusive, while the scissor covers pixels from (0, 0) to (638, 478) inclusive. This means that pixels with `x=639` or `y=479` never get drawn, and remain black. (Note that this is actually the second scissor the game uses; when it draws shadows, it uses a smaller scissor and viewport which is not affected by the bug.) When the texture is drawn, an indirect texture is used to adjust texture coordinates (the details of which vary depending on the effect the game is rendering, and I go into this more in the other PBR document). The base texture, which is an earlier EFB copy, has clamping enabled. Thus, if the indirect effect causes texture coordinates to go outside of the texture, they end up inside of the texture (instead of being wrapped): `x < 0` becomes `x = 0`, `x > 639` becomes `639`, `y < 0` becomes `0`, `y > 479` becomes `479`. You may see the problem already: `x = 639` and `y = 479` are black due to the scissor, and thus everything beyond them is also black, meaning the indirect effects can bring in large black regions. (For instance, if `x` is already 630, and the indirect texture would result in adding 15 to x, then the result is blackness.) -
Pokechu22 revised this gist
Dec 20, 2021 . 2 changed files with 2 additions and 2 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -4,7 +4,7 @@ This writeup mainly analyzes the [psycho cut fifolog](https://fifo.ci/media/dff/ ## How the effects render All of the effects work using indirect textures. First, the game draws the main environment (see [ZPBR_Psy_base_new.png](#file-zpbr_psy_base_new-png) and [ZPBR_Rain_base_new.png](#file-zpbr_rain_base_new-png)). Then, it makes an EFB copy, and clears the screen (but not the depth buffer). It then draws a second effect, which will serve as an offset to the screen (see [ZPBR_Psy_indirect.png](#file-zpbr_psy_indirect-png) and [ZPBR_Rain_indirect.png](#file-zpbr_rain_indirect-png)). This might be drawn in 3D space (as with the energy pattern used by Psycho Cut) or it might be drawn in 2D space (as with the rain effect); that depends on whether the effect remain still in 3D space while the camera moves (e.g. the energy pattern) or should move with the camera (e.g. the rain effect, which acts as water _on_ the camera). Since the depth buffer was not cleared, the effects will not show through solid objects (this is visible in the Psycho Cut case, as the effect actually would continue underneath the floor, but it gets cut off, producing a visible edge at the bottom - note that the energy effect itself does not have depth updates enabled). All of this means that the game can adjust the camera and the effects still working properly with no additional CPU-side computation, and also means that Dolphin's free-look functionality can work properly with it. After that effect has been prepared, another EFB copy is made, and then both the original and new EFB copies are combined. <details><summary>EFB copy 3 (0005aaf3)</summary> This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -8,7 +8,7 @@ Specifically, the rightmost column and bottommost row of pixels are all black du When the texture is drawn, an indirect texture is used to adjust texture coordinates (the details of which vary depending on the effect the game is rendering, and I go into this more in the other PBR document). The base texture, which is an earlier EFB copy, has clamping enabled. Thus, if the indirect effect causes texture coordinates to go outside of the texture, they end up inside of the texture (instead of being wrapped): `x < 0` becomes `x = 0`, `x > 639` becomes `639`, `y < 0` becomes `0`, `y > 479` becomes `479`. You may see the problem already: `x = 639` and `y = 479` are black due to the scissor, and thus everything beyond them is also black, meaning the indirect effects can bring in large black regions. (For instance, if `x` is already 630, and the indirect texture would result in adding 15 to x, then the result is blackness.) The fix is to adjust the scissor rectangle so that it actually covers the whole screen (by adding 1 to the bottom and right coordinates in this case). Then, the clamping works properly: `x = 639` and `y = 479` both look similar to their neigboring pixels, instead of being completely black. Note that this doesn't magically add additional detail when the indirect texture results in going beyond the edge of the texture; this is easiest to see on the right-hand side of [the rain output image](#file-zpbr_rain_out_both-png) where the checkerboard does not continue within the rain effect (but it's a good enough approximation that it doesn't look _wrong_ at first glance either, which is way better than black). Note also that `x = 0` and `y = 0` already worked properly with clamping (however, the effects in Pokémon Battle Revolution mainly shift down/left and not up/right, so this doesn't mean much in practice). ## The code -
Pokechu22 revised this gist
Dec 19, 2021 . 1 changed file with 1 addition and 1 deletion.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -50,7 +50,7 @@ void GSrender_SetScissor(GSrender *render,uint x,uint y,uint width,uint height) } ``` The `register0x00000004` stuff is related to an `addic.` instruction (add immediate with carry, and then update the status register) on the stack pointer, and that `screenWidth`/`scissorHeight` are not set if the result of the add is 0. However, there is no valid reason for the stack pointer to be near 0, so that check really shouldn't exist (and this same function will break in other places if the stack pointer is invalid). My best guess is that this is a case of the compiler being overly paranoid or just buggy. The name itself is a bit odd, but note that `register0x00000004` could also be labeled as `unaff_r1`; see [this Ghidra bug report](https://github.com/NationalSecurityAgency/ghidra/issues/3771). In practice, I think the code is actually something like this: -
Pokechu22 revised this gist
Dec 19, 2021 . 1 changed file with 1 addition and 1 deletion.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -363,7 +363,7 @@ EFB copy 4 uses the IA8 format, an intensity format. (The EFB copy's target pix The game then draws over the entire screen using a rectangle where the texture coordinate ranges from (0, 0) at the top-left and (1, 1) at the bottom-right. This texture coordinate is used for two textures at the same time: the one corresponding to EFB copy 3, and the one corresponding to EFB copy 4. The texture coordinate is multiplied by the corresponding size info, causing it to range from (0, 0) to (640, 480). It then is used to sample the texture for EFB copy 4, as part of the indirect stage; however, it is scaled down by a factor of 2 for this (as set in `BPMEM_RAS1_SS0`), which is needed because EFB copy 4 is half-size (thus, the scaled texture coordinate ranges from (0, 0) to (320, 240), which is what is needed for EFB copy 4). EFB copy 4 is used as an indirect texture, meaning the color read from it is multiplied with the indirect matrix, and then that product is added back to the original texture coordinate (not the one that has been scaled by a factor of 2). The specific indirect matrix used has the same values for both rows, meaning that both the `s` and `t` texture coordinates are offset by the same amount (effectively resulting in brighter values in EFB copy 4 resulting in an pixels using texture coordinates that are further diagonally down-right, resulting in the overall image moving up-left). The modified texture coordinate is then used to sample the texture for EFB copy 3. A somewhat trivial TEV configuration is used where the output color is simply set to the sampled color from the texture for EFB copy 3, and the output alpha value is set to 0. Since the EFB has no alpha channel (and the alpha test is configured to always pass), the value for alpha here does not matter. -
Pokechu22 revised this gist
Dec 19, 2021 . 2 changed files with 11 additions and 11 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -2,7 +2,7 @@ Pokémon Battle Revolution had an issue where certain effects (those that distor This writeup mainly analyzes the [psycho cut fifolog](https://fifo.ci/media/dff/psycho_cut_fifo.dff) (from [issue 12629](https://bugs.dolphin-emu.org/issues/12629), though the actual issue is described in [issue 11875](https://bugs.dolphin-emu.org/issues/11875)). ## How the effects render All of the effects work using indirect textures. First, the game draws the main environment (see [ZPBR_Psy_base_new.png](#file-ZPBR_Psy_base_new-png) and [ZPBR_Rain_base_new.png](#file-ZPBR_Rain_base_new-png)). Then, it makes an EFB copy, and clears the screen (but not the depth buffer). It then draws a second effect, which will serve as an offset to the screen (see [ZPBR_Psy_indirect.png](#file-ZPBR_Psy_indirect-png) and [ZPBR_Rain_indirect.png](#file-ZPBR_Rain_indirect-png)). This might be drawn in 3D space (as with the energy pattern used by Psycho Cut) or it might be drawn in 2D space (as with the rain effect); that depends on whether the effect remain still in 3D space while the camera moves (e.g. the energy pattern) or should move with the camera (e.g. the rain effect, which acts as water _on_ the camera). Since the depth buffer was not cleared, the effects will not show through solid objects (this is visible in the Psycho Cut case, as the effect actually would continue underneath the floor, but it gets cut off, producing a visible edge at the bottom - note that the energy effect itself does not have depth updates enabled). All of this means that the game can adjust the camera and the effects still working properly with no additional CPU-side computation, and also means that Dolphin's free-look functionality can work properly with it. After that effect has been prepared, another EFB copy is made, and then both the original and new EFB copies are combined. @@ -105,6 +105,8 @@ Automatic color conversion: Yes <details><summary>Object 440</summary> <details><summary>Indirect matrices (0005c778)</summary> The indirect scale comes out as 17, which means that the effective scale is `2^(17-17) = 2^0 = 1`. In other words, the matrix entries can be used directly. ``` BP register BPMEM_IND_MTXA Matrix 0 Matrix 0 column A @@ -125,8 +127,6 @@ Row 1 (mf): 0 (0) Scale bits: 1 (shifted: 16), given to SDK as 1 (16) ``` </details> <details><summary>TEV configuration (0005c7a9)</summary> @@ -341,6 +341,8 @@ ZFreeze: No </details> <details><summary>Primitive data (0005c8fb)</summary> This draws a rectangle covering the entire screen (from (0, 0) to (640, 480)) with a single texture coordinate ranging from (0, 0) to (1, 1). ``` Primitive GX_DRAW_TRIANGLE_STRIP (3) VAT 1 @@ -350,8 +352,6 @@ Primitive GX_DRAW_TRIANGLE_STRIP (3) VAT 1 44200000 (640) 43f00000 (480) 3f800000 (1) 3f800000 (1) ``` </details> </details> @@ -395,7 +395,7 @@ void main(void) { } ``` ## The bug The indirect texture matrix is `s = t = -0.036132812 * a + 0.58203125 * r + 0 * g`, where `a` is the alpha channel and `r`/`g`/`b` are all the value of the intensity channel (`b` is not available to the indirect matrix, however). In other It may seem odd that alpha appears in this equation, given that the EFB does not have an alpha channel. Indeed, that's the source of the problem: Dolphin was using an alpha value of 0 in this case, when it should have been using an alpha value of 255 (it already used an alpha value of 255 for non-intensity formats, but was using 0 for intensity formats). This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -2,15 +2,15 @@ In addition to the shifting issue, Pokémon Battle Revolution has a black border This document looks at both the psycho cut fifolog and a rain fifolog, both of which are on [issue 12629](https://bugs.dolphin-emu.org/issues/12629). However, specific object numbers aren't relevant for this issue. ## The problem Specifically, the rightmost column and bottommost row of pixels are all black due to a poorly-configured scissor rectangle. The game has the scissor top-left at (342, 342), bottom-right at (980, 820), scissor offset of (342, 342), and the viewport covers (342, 342) to (982, 822). Without going into details on the calculation, this means that the viewport covers pixels from (0, 0) to (639, 479) inclusive, while the scissor covers pixels from (0, 0) to (638, 479) inclusive. This means that pixels with `x=639` or `y=479` never get drawn, and remain black. (Note that this is actually the second scissor the game uses; when it draws shadows, it uses a smaller scissor and viewport which is not affected by the bug.) When the texture is drawn, an indirect texture is used to adjust texture coordinates (the details of which vary depending on the effect the game is rendering, and I go into this more in the other PBR document). The base texture, which is an earlier EFB copy, has clamping enabled. Thus, if the indirect effect causes texture coordinates to go outside of the texture, they end up inside of the texture (instead of being wrapped): `x < 0` becomes `x = 0`, `x > 639` becomes `639`, `y < 0` becomes `0`, `y > 479` becomes `479`. You may see the problem already: `x = 639` and `y = 479` are black due to the scissor, and thus everything beyond them is also black, meaning the indirect effects can bring in large black regions. (For instance, if `x` is already 630, and the indirect texture would result in adding 15 to x, then the result is blackness.) The fix is to adjust the scissor rectangle so that it actually covers the whole screen (by adding 1 to the bottom and right coordinates in this case). Then, the clamping works properly: `x = 639` and `y = 479` both look similar to their neigboring pixels, instead of being completely black. Note that this doesn't magically add additional detail when the indirect texture results in going beyond the edge of the texture; this is easiest to see on the right-hand side of [the rain output image](#file-ZPBR_Rain_out_both-png) where the checkerboard does not continue within the rain effect (but it's a good enough approximation that it doesn't look _wrong_ at first glance either, which is way better than black). Note also that `x = 0` and `y = 0` already worked properly with clamping (however, the effects in Pokémon Battle Revolution mainly shift down/left and not up/right, so this doesn't mean much in practice). ## The code Here is the relevant code, as generated by Ghidra with some naming but no cleanup (this code is located at `80244a4c` in the NTSC-U version of the game): @@ -102,7 +102,7 @@ The easiest fix is to just stop subtracting 1 from `screenWidth` and `screenHeig But now, `GSrender_SetScissor(render, 1000, 1000, 4, 4)` would result in a call of `GXSetScissor(640, 480, 0, 0)` - is that a problem? No, as before that same call would have resulted in `GXSetScissor(639, 479, 0, 0)` which affects on-screen pixels even though the scissor is off-screen. An exclusive width of 0 also seems like it would cause issues, but in practice, if the top-left coordinate is past the bottom-right coordinate, nothing is rendered (this was checked on real hardware). Theoretically, none of this clamping is needed at all, as the scissor functionality works mostly fine with out-of-bounds ranges (as long as the ranges are close enough to in bounds that nothing overflows), but there's no harm in having it. ## The patch As determined above, the fix is simply to get rid of the code that subtracts 1 from `screenWidth`/`screenHeight`. This is pretty easy: we just need to replace two instructions: @@ -139,7 +139,7 @@ $Fix bottom/right pixels being black and black screen effects [Pokechu22] It's worth noting that this code fixes the issue on real hardware — again, this isn't a Dolphin-specific problem, but a game bug. ## Patching other versions of the game I only own a USA copy of Pokémon Battle Revolution, so I only have a patch for it. Furthermore, the code in various versions of the game will almost certainly be at slightly different addresses, meaning the same patch won't work on all versions of the game. However, it should be possible to generate patches for other versions without as much trouble by following these steps: @@ -155,7 +155,7 @@ I only own a USA copy of Pokémon Battle Revolution, so I only have a patch for 10. Start scrolling down until you see `subi r8, r8, 1`. There should be a `subi r0, r3, 1` a few instructions below too. 11. For each instruction, right-click and copy the address, and then also copy the hex. The hex should end in `ffff`; changing it to `0000` will solve the issue. A 32-bit memory patch is fine for this. ## Note about dolphin Note that when generating images, I used a modified version of Dolphin so that I could use the same fifolog for both images. Here is the change I made: -
Pokechu22 revised this gist
Dec 19, 2021 . 1 changed file with 62 additions and 4 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -6,8 +6,6 @@ This writeup mainly analyzes the [psycho cut fifolog](https://fifo.ci/media/dff/ All of the effects work using indirect textures. First, the game draws the main environment (see [ZPBR_Psy_base_new.png](#file-ZPBR_Psy_base_new-png) and [ZPBR_Rain_base_new.png](#file-ZPBR_Rain_base_new-png)). Then, it makes an EFB copy, and clears the screen (but not the depth buffer). It then draws a second effect, which will serve as an offset to the screen (see [ZPBR_Psy_indirect.png](#file-ZPBR_Psy_indirect-png) and [ZPBR_Rain_indirect.png](#file-ZPBR_Rain_indirect-png)). This might be drawn in 3D space (as with the energy pattern used by Psycho Cut) or it might be drawn in 2D space (as with the rain effect); that depends on whether the effect remain still in 3D space while the camera moves (e.g. the energy pattern) or should move with the camera (e.g. the rain effect, which acts as water _on_ the camera). Since the depth buffer was not cleared, the effects will not show through solid objects (this is visible in the Psycho Cut case, as the effect actually would continue underneath the floor, but it gets cut off, producing a visible edge at the bottom - note that the energy effect itself does not have depth updates enabled). All of this means that the game can adjust the camera and the effects still working properly with no additional CPU-side computation, and also means that Dolphin's free-look functionality can work properly with it. After that effect has been prepared, another EFB copy is made, and then both the original and new EFB copies are combined. <details><summary>EFB copy 3 (0005aaf3)</summary> ``` @@ -40,6 +38,9 @@ BP register BPMEM_EFB_WH EFB Width: 640 EFB Height: 480 BP register BPMEM_MIPMAP_STRIDE (0x140) No description available BP register BPMEM_EFB_ADDR EFB Target address (32 byte aligned): 0xA44A20 @@ -80,6 +81,9 @@ BP register BPMEM_EFB_WH EFB Width: 640 EFB Height: 480 BP register BPMEM_MIPMAP_STRIDE (0x50) No description available BP register BPMEM_EFB_ADDR EFB Target address (32 byte aligned): 0xB71520 @@ -334,17 +338,71 @@ Num indirect stages: 1 ZFreeze: No ``` </details> <details><summary>Primitive data (0005c8fb)</summary> ``` Primitive GX_DRAW_TRIANGLE_STRIP (3) VAT 1 00000000 (0) 00000000 (0) 00000000 (0) 00000000 (0) 44200000 (640) 00000000 (0) 3f800000 (1) 00000000 (0) 00000000 (0) 43f00000 (480) 00000000 (0) 3f800000 (1) 44200000 (640) 43f00000 (480) 3f800000 (1) 3f800000 (1) ``` This draws a rectangle covering the entire screen (from (0, 0) to (640, 480)) with a single texture coordinate ranging from (0, 0) to (1, 1). </details> </details> Pokémon Battle Revolution uses an RGB8 EFB (meaning each framebuffer pixel has 8 bits of red, green, and blue data, and no alpha data is stored at all). Object 68, offset 0002c85a (not shown below) sets the pixel format in this case, though the same value is used in several places before and after. The first relevant EFB copy is EFB copy 3, right before object 423; the second relevant one is EFB copy 4, right before object 440. Object 440 then combines the two, applying the indirect effect. EFB copy 3 uses the RGBA8 format, and captures the whole screen as a 640 by 480 texture. RGBA8 indicates that there are 8 bits of data each for red, green, blue, and alpha color channels. Note that since this format includes an alpha channel, but the EFB does not, some default value needs to be used for it. This value is 1.0 (or 255). EFB copy 4 uses the IA8 format, an intensity format. (The EFB copy's target pixel format is 3, which Dolphin labels as "RA8/IA8 (Z16 too?)"; it also has intensity format set to true.) This EFB copy captures the whole screen as a 320 by 240 texture, because it has half-scale enabled. The scaling down by 2 is presumably only to reduce memory usage; it doesn't seem to be necessary for rendering. IA8 indicates that the texture has an intensity channel and an alpha channel. This means that the system needs to map the EFB's red, green, and blue channels into a single channel, which is a somewhat complicated process I won't go into here. When rendering, the intensity channel is used as the value for the red, green, and blue channels (though that doesn't apply in this case, as EFB copy 4 isn't rendered directly to the screen). As with before, there's also an alpha channel, which needs to have some default value. The game then draws over the entire screen using a rectangle where the texture coordinate ranges from (0, 0) at the top-left and (1, 1) at the bottom-right. This texture coordinate is used for two textures at the same time: the one corresponding to EFB copy 3, and the one corresponding to EFB copy 4. The texture coordinate is multiplied by the corresponding size info, causing it to range from (0, 0) to (640, 480). It then is used to sample the texture for EFB copy 4, as part of the indirect stage; however, it is scaled down by a factor of 2 for this (as set in `BPMEM_RAS1_SS0`), which is needed because EFB copy 4 is half-size (thus, the scaled texture coordinate ranges from (0, 0) to (320, 240), which is what is needed for EFB copy 4). EFB copy 4 is used as an indirect texture, meaning the color read from it is multiplied with the indirect matrix, and then that product is added back to the original texture coordinate (not the one that has been scaled by a factor of 2). The specific indirect matrix used has the same values for both rows, meaning that both the `s` and `t` texture coordinates are offset by the same amount (effectively resulting in brighter values in EFB copy 4 resulting in a shift diagonally down-right). The modified texture coordinate is then used to sample the texture for EFB copy 3. A somewhat trivial TEV configuration is used where the output color is simply set to the sampled color from the texture for EFB copy 3, and the output alpha value is set to 0. Since the EFB has no alpha channel (and the alpha test is configured to always pass), the value for alpha here does not matter. A somewhat simplified version of what the game is doing (in pseudo-glsl-that-probably-has-compile-errors™) is this: ```glsl uniform sampler2D base_texture; // EFB copy 3 uniform sampler2D indirect_texture; // EFB copy 4 uniform mat3x2 ind_mtx; varying vec2 texture_coordinate; // from (0, 0) to (1, 1) void main(void) { texture_coordinate.u *= 640; texture_coordinate.v *= 480; vec2 indirect_sample_coord = texture_coordinate; indirect_sample_coord.u /= 2; indirect_sample_coord.v /= 2; vec4 color = texture(indirect_texture, indirect_sample_coord); // color.r == color.g == color.b; all are set to the intensity channel of the indirect texture texture_coordinate.u += ind_mtx[0][0] * color.a + ind_mtx[1][0] * color.r + ind_mtx[2][0] * color.g; texture_coordinate.v += ind_mtx[0][1] * color.a + ind_mtx[1][1] * color.r + ind_mtx[2][1] * color.g; // i.e. texture_coordinate += ind_mtx * color.arg; gl_FragColor = vec4(texture(base_texture, texture_coordinate).rgb, 0); } ``` # The bug The indirect texture matrix is `s = t = -0.036132812 * a + 0.58203125 * r + 0 * g`, where `a` is the alpha channel and `r`/`g`/`b` are all the value of the intensity channel (`b` is not available to the indirect matrix, however). In other It may seem odd that alpha appears in this equation, given that the EFB does not have an alpha channel. Indeed, that's the source of the problem: Dolphin was using an alpha value of 0 in this case, when it should have been using an alpha value of 255 (it already used an alpha value of 255 for non-intensity formats, but was using 0 for intensity formats). But why is the alpha channel needed? Well, what they're actually trying to do is add a constant offset, and the only way to do that is to use a constant value. Specifically, since alpha is 255, the offset is `-9.21386706`. But why would that offset be wanted? It seems rather arbitrary... The answer is that intensity formats use the [BT.601 standard](https://en.wikipedia.org/wiki/Rec._601), and that standard doesn't use a value of 0 for black and 255 for white. Instead, black is 16, and white is 235. (This can be seen in the indirect images.) So, if black is not supposed to perform any offsetting, that 16 needs to be converted back to zero. It just so happens that `-0.036132812 * 255 + 0.58203125 * 16 = -9.21386706 + 9.3125 = 0.09863294`, or about 1/10th of a pixel in offset, which is the best that is achievable with the values you can use in the indirect matrix. Similarly, `(235 - 16)*0.58203125` is `127.46484375`, the closest value to `127.5 = 255/2` that can be achieved. So, these strange-looking values are trying to convert to the range of `[0, 127.5]`. But if the alpha channel has the wrong value, it instead ends up being a range of approximately `[-9.2, 118.3]`, meaning pixels that were not supposed to be shifted at all were instead shifted by 9 pixels both horizontally and vertically. There is still a slight shift (of `0.09863294` pixels) left over, but that shift would happen on real hardware too, and there's no practical way to eliminate it. But, given that the camera usually moves by more than that each frame, I don't think that shift is noticeable in practice, even at higher IRs where 1/10th of a pixel becomes 1 pixel. As a side note, the original shift issue results in a black border. Why is it black? Well, that's actually a separate issue, caused by a game bug. See the next document for more information... -
Pokechu22 revised this gist
Dec 18, 2021 . 1 changed file with 0 additions and 5 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -1,5 +0,0 @@ -
Pokechu22 revised this gist
Dec 18, 2021 . 1 changed file with 12 additions and 0 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -336,3 +336,15 @@ ZFreeze: No </details> </details> TODO: more explanation # The bug The indirect texture matrix is `s = t = -0.036132812 * a + 0.58203125 * r + 0 * g`, where `a` is the alpha channel and `r`/`g`/`b` are all the intensity channel (`b` is not available to the indirect matrix, however). In other It may seem odd that alpha appears in this equation, given that the EFB does not have an alpha channel. Indeed, that's the source of the problem: Dolphin was using an alpha value of 0 in this case, when it should have been using an alpha value of 255 (it already used an alpha value of 255 for non-intensity formats, but was using 0 for intensity formats). But why is the alpha channel needed? Well, what they're actually trying to do is add a constant offset, and the only way to do that is to use a constant value. Specifically, since alpha is 255, the offset is `-9.21386706`. But why would that offset be wanted? It seems rather arbitrary... The answer is that intensity formats use the [BT.601 standard](https://en.wikipedia.org/wiki/Rec._601), and that standard doesn't use a value of 0 for black and 255 for white. Instead, black is 16, and white is 235. (This can be seen in the indirect images.) So, if black is not supposed to perform any offsetting, that 16 needs to be converted back to zero. It just so happens that `-0.036132812 * 255 + 0.58203125 * 16 = -9.21386706 + 9.3125 = 0.09863294`, or about 1/10th of a pixel in offset, which is the best that is achievable with the values you can use in the indirect matrix. Similarly, `(235 - 16)*0.58203125` is `127.46484375`, the closest value to `127.5 = 255/2` that can be achieved. So, these strange-looking values are trying to convert to the range of `[0, 127.5]`. But if the alpha channel has the wrong value, it instead ends up being a range of approximately `[-9.2, 118.3]`, meaning pixels that were not supposed to be shifted at all were instead shifted by 9 pixels both horizontally and vertically. There is still a slight shift (of `0.09863294` pixels) left over, but that shift would happen on real hardware too, and there's no practical way to eliminate it. But, given that the camera usually moves by more than that each frame, I don't think that shift is noticeable in practice, even at higher IRs where 1/10th of a pixel becomes 1 pixel. -
Pokechu22 revised this gist
Dec 18, 2021 . 1 changed file with 1 addition and 1 deletion.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -8,7 +8,7 @@ Specifically, the rightmost column and bottommost row of pixels are all black du When the texture is drawn, an indirect texture is used to adjust texture coordinates (the details of which vary depending on the effect the game is rendering, and I go into this more in the other PBR document). The base texture, which is an earlier EFB copy, has clamping enabled. Thus, if the indirect effect causes texture coordinates to go outside of the texture, they end up inside of the texture (instead of being wrapped): `x < 0` becomes `x = 0`, `x > 639` becomes `639`, `y < 0` becomes `0`, `y > 479` becomes `479`. You may see the problem already: `x = 639` and `y = 479` are black due to the scissor, and thus everything beyond them is also black, meaning the indirect effects can bring in large black regions. (For instance, if `x` is already 630, and the indirect texture would result in adding 15 to x, then the result is blackness.) The fix is to adjust the scissor rectangle so that it actually covers the whole screen (by adding 1 to the bottom and right coordinates in this case). Then, the clamping works properly: `x = 639` and `y = 479` both look similar to their neigboring pixels, instead of being completely black. Note that this doesn't magically add additional detail when the indirect texture results in going beyond the edge of the texture; this is easiest to see on the right-hand side of [the rain output image](#file-ZPBR_Rain_out_both-png) where the checkerboard does not continue within the rain effect (but it's a good enough approximation that it doesn't look _wrong_ at first glance either, which is way better than black). Note also that `x = 0` and `y = 0` already worked properly with clamping (however, the effects in Pokémon Battle Revolution mainly shift down/left and not up/right, so this doesn't mean much in practice). # The code -
Pokechu22 revised this gist
Dec 18, 2021 . 1 changed file with 302 additions and 3 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -4,13 +4,34 @@ This writeup mainly analyzes the [psycho cut fifolog](https://fifo.ci/media/dff/ # How the effects render All of the effects work using indirect textures. First, the game draws the main environment (see [ZPBR_Psy_base_new.png](#file-ZPBR_Psy_base_new-png) and [ZPBR_Rain_base_new.png](#file-ZPBR_Rain_base_new-png)). Then, it makes an EFB copy, and clears the screen (but not the depth buffer). It then draws a second effect, which will serve as an offset to the screen (see [ZPBR_Psy_indirect.png](#file-ZPBR_Psy_indirect-png) and [ZPBR_Rain_indirect.png](#file-ZPBR_Rain_indirect-png)). This might be drawn in 3D space (as with the energy pattern used by Psycho Cut) or it might be drawn in 2D space (as with the rain effect); that depends on whether the effect remain still in 3D space while the camera moves (e.g. the energy pattern) or should move with the camera (e.g. the rain effect, which acts as water _on_ the camera). Since the depth buffer was not cleared, the effects will not show through solid objects (this is visible in the Psycho Cut case, as the effect actually would continue underneath the floor, but it gets cut off, producing a visible edge at the bottom - note that the energy effect itself does not have depth updates enabled). All of this means that the game can adjust the camera and the effects still working properly with no additional CPU-side computation, and also means that Dolphin's free-look functionality can work properly with it. After that effect has been prepared, another EFB copy is made, and then both the original and new EFB copies are combined. Pokémon Battle Revolution uses an RGB8 EFB (meaning each framebuffer pixel has 8 bits of red, green, and blue data, and no alpha data is stored at all). Object 68, offset 0002c85a (not shown below) sets the pixel format in this case, though the same value is used in several places before and after. The first relevant EFB copy is EFB copy 3, right before object 423; the second relevant one is EFB copy 4, right before object 440. Object 440 then combines the two. applying the indirect effect. <details><summary>EFB copy 3 (0005aaf3)</summary> ``` BP register BPMEM_CLEAR_AR Clear color alpha: 0x00 Clear color red: 0x00 BP register BPMEM_CLEAR_GB Clear color green: 0x00 Clear color blue: 0x00 BP register BPMEM_CLEAR_Z Clear Z value: 0xFFFFFF BP register BPMEM_ZMODE Z mode: Enable test: Yes Compare function: Always (7) Enable updates: No BP register BPMEM_ZCOMPARE EFB pixel format: RGB8_Z24 (0) Depth format: linear (0) Early depth test: No BP register BPMEM_EFB_TL EFB Left: 0 EFB Top: 0 @@ -37,3 +58,281 @@ Automatic color conversion: Yes ``` </details> <details><summary>EFB copy 4 (0005c726)</summary> ``` BP register BPMEM_CLEAR_AR Clear color alpha: 0x00 Clear color red: 0x00 BP register BPMEM_CLEAR_GB Clear color green: 0x00 Clear color blue: 0x00 BP register BPMEM_CLEAR_Z Clear Z value: 0xFFFFFF BP register BPMEM_EFB_TL EFB Left: 0 EFB Top: 0 BP register BPMEM_EFB_WH EFB Width: 640 EFB Height: 480 BP register BPMEM_EFB_ADDR EFB Target address (32 byte aligned): 0xB71520 BP register BPMEM_TRIGGER_EFB_COPY Clamping: Top and Bottom Converting from RGB to YUV: No Target pixel format: RA8/IA8 (Z16 too?) (3) Gamma correction: 1.0 Half scale: Yes Vertical scaling: No Clear: No Frame to field: Progressive (0) Copy to XFB: No Intensity format: Yes Automatic color conversion: Yes ``` </details> <details><summary>Object 440</summary> <details><summary>Indirect matrices (0005c778)</summary> ``` BP register BPMEM_IND_MTXA Matrix 0 Matrix 0 column A Row 0 (ma): -0.036132812 (-37) Row 1 (mb): -0.036132812 (-37) Scale bits: 1 (shifted: 1) BP register BPMEM_IND_MTXB Matrix 0 Matrix 0 column B Row 0 (mc): 0.58203125 (596) Row 1 (md): 0.58203125 (596) Scale bits: 0 (shifted: 0) BP register BPMEM_IND_MTXC Matrix 0 Matrix 0 column C Row 0 (me): 0 (0) Row 1 (mf): 0 (0) Scale bits: 1 (shifted: 16), given to SDK as 1 (16) ``` The indirect scale comes out as 17, which means that the effective scale is `2^(17-17) = 2^0 = 1`. In other words, the matrix entries can be used directly. </details> <details><summary>TEV configuration (0005c7a9)</summary> ``` BP register BPMEM_TREF number 0 Stage 0 texmap: 0 Stage 0 tex coord: 0 Stage 0 enable texmap: Yes Stage 0 color channel: Zero (7) Stage 1 texmap: 0 Stage 1 tex coord: 0 Stage 1 enable texmap: Yes Stage 1 color channel: Color chan 1 (1) -Duplicate half-configured configuration skipped- BP register BPMEM_TEV_COLOR_ENV Tev stage 0 dest.rgb = tex.rgb a: ZERO (15) b: ZERO (15) c: ZERO (15) d: tex.rgb (8) Bias: 0 (0) Op: Add (0) / Comparison: Greater than (0) Clamp: Yes Scale factor: 1 (0) / Compare mode: R8 (0) Dest: prev (0) BP register BPMEM_TEV_ALPHA_ENV Tev stage 0 dest.a = 0 a: ZERO (7) b: ZERO (7) c: ZERO (7) d: ZERO (7) Bias: 0 (0) Op: Add (0) / Comparison: Greater than (0) Clamp: Yes Scale factor: 1 (0) / Compare mode: R8 (0) Dest: prev (0) Ras sel: 0 Tex sel: 0 BP register BPMEM_IND_CMD command 0 Indirect tex stage ID: 0 Format: ITF_8 (0) Bias: None (0) Bump alpha: Off (0) Offset matrix index: Matrix 0 (1) Offset matrix ID: Indirect (0) Regular coord S wrapping factor: Off (0) Regular coord T wrapping factor: Off (0) Use modified texture coordinates for LOD computation: No Add texture coordinates from previous TEV stage: No BP register BPMEM_IREF Stage 0 ntexmap: 1 Stage 0 ntexcoord: 0 Stage 1 ntexmap: 0 Stage 1 ntexcoord: 0 Stage 2 ntexmap: 0 Stage 2 ntexcoord: 0 Stage 3 ntexmap: 0 Stage 3 ntexcoord: 0 BP register BPMEM_RAS1_SS0 Indirect texture stages 0 and 1: Even stage S scale: 1 (0.5) Even stage T scale: 1 (0.5) Odd stage S scale: 0 (1) Odd stage T scale: 0 (1) ``` </details> <details><summary>Texture 0 configuration (0005c7d6)</summary> Texture 0 points to EFB copy 3. ``` BP register BPMEM_TX_SETMODE0 Texture Unit 0 Wrap S: Clamp (0) Wrap T: Clamp (0) Mag filter: Linear (1) Mipmap filter: None (0) Min filter: Linear (1) LOD type: Diagonal LOD (1) LOD bias: 0 (0) Max anisotropic filtering: 1 (0) LOD/bias clamp: No BP register BPMEM_TX_SETMODE1 Texture Unit 0 Min LOD: 0 (0) Max LOD: 0 (0) BP register BPMEM_TX_SETIMAGE0 Texture Unit 0 Width: 640 Height: 480 Format: RGBA8 (6) BP register BPMEM_TX_SETIMAGE1 Texture Unit 0 Even TMEM Offset: 0 Even TMEM Width: 3 Even TMEM Height: 3 Cache is manually managed: No BP register BPMEM_TX_SETIMAGE2 Texture Unit 0 Odd TMEM Offset: 4000 Odd TMEM Width: 3 Odd TMEM Height: 3 BP register BPMEM_TX_SETIMAGE3 Texture Unit 0 Source address (32 byte aligned): 0xA44A20 ``` </details> <details><summary>Texture 1 configuration (0005c7f4)</summary> Texture 1 points to EFB copy 4. ``` BP register BPMEM_TX_SETMODE0 Texture Unit 1 Wrap S: Clamp (0) Wrap T: Clamp (0) Mag filter: Linear (1) Mipmap filter: None (0) Min filter: Linear (1) LOD type: Diagonal LOD (1) LOD bias: 0 (0) Max anisotropic filtering: 1 (0) LOD/bias clamp: No BP register BPMEM_TX_SETMODE1 Texture Unit 1 Min LOD: 0 (0) Max LOD: 0 (0) BP register BPMEM_TX_SETIMAGE0 Texture Unit 1 Width: 320 Height: 240 Format: IA8 (3) BP register BPMEM_TX_SETIMAGE1 Texture Unit 1 Even TMEM Offset: 800 Even TMEM Width: 3 Even TMEM Height: 3 Cache is manually managed: No BP register BPMEM_TX_SETIMAGE2 Texture Unit 1 Odd TMEM Offset: c00 Odd TMEM Width: 3 Odd TMEM Height: 3 BP register BPMEM_TX_SETIMAGE3 Texture Unit 1 Source address (32 byte aligned): 0xB71520 ``` </details> <details><summary>Texture coordinate scale configuration (0005c870)</summary> This is configured twice; only the second one actually applies. (The configuration is based on a texture size, though actual hardware doesn't care about the texture corresponding to the coordinate.) ``` BP register BPMEM_SU_SSIZE number 0 S size info: Scale: 320 Range bias: No Cylindric wrap: No Use line offset: No (s only) Use point offset: No (s only) BP register BPMEM_SU_TSIZE number 0 T size info: Scale: 240 Range bias: No Cylindric wrap: No Use line offset: No (s only) Use point offset: No (s only) BP register BPMEM_SU_SSIZE number 0 S size info: Scale: 640 Range bias: No Cylindric wrap: No Use line offset: No (s only) Use point offset: No (s only) BP register BPMEM_SU_TSIZE number 0 T size info: Scale: 480 Range bias: No Cylindric wrap: No Use line offset: No (s only) Use point offset: No (s only) ``` </details> <details><summary>Genmode (0005c884)</summary> ``` BP register BPMEM_GENMODE Num tex gens: 1 Num color channels: 0 Unused bit: 0 Flat shading (unconfirmed): No Multisampling: No Num TEV stages: 1 Cull mode: Back-facing primitives only (1) Num indirect stages: 1 ZFreeze: No ``` </details> </details> -
Pokechu22 revised this gist
Dec 18, 2021 . 2 changed files with 41 additions and 0 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -0,0 +1,39 @@ Pokémon Battle Revolution had an issue where certain effects (those that distort the camera) caused the entire screen to shift slightly. This writeup mainly analyzes the [psycho cut fifolog](https://fifo.ci/media/dff/psycho_cut_fifo.dff) (from [issue 12629](https://bugs.dolphin-emu.org/issues/12629), though the actual issue is described in [issue 11875](https://bugs.dolphin-emu.org/issues/11875)). # How the effects render All of the effects work using indirect textures. First, the game draws the main environment (see [ZPBR_Psy_base_new.png](#file-ZPBR_Psy_base_new-png) and [ZPBR_Rain_base_new.png](#file-ZPBR_Rain_base_new-png)). Then, it makes an EFB copy, and clears the screen. It then draws a second effect, which will serve as an offset to the screen (see [ZPBR_Psy_indirect.png](#file-ZPBR_Psy_indirect-png) and [ZPBR_Rain_indirect.png](#file-ZPBR_Rain_indirect-png)). This might be drawn in 3D space (as with the energy pattern used by Psycho Cut) or it might be drawn in 2D space (as with the rain effect); that depends on whether the effect remain still in 3D space while the camera moves (e.g. the energy pattern) or should move with the camera (e.g. the rain effect, which acts as water _on_ the camera). This means that the game can adjust the camera and the effects still working properly with no additional CPU-side computation, and also means that Dolphin's free-look functionality can work properly with it. After that effect has been prepared, another EFB copy is made, and then both the original and new EFB copies are combined. Pokémon Battle Revolution uses an RGB8 EFB (meaning each framebuffer pixel has 8 bits of red, green, and blue data, and no alpha data is stored at all). (In the psycho cut fifolog, see object 68, offset 0002c85a (BPMEM_ZCOMPARE), and object 423, offset 0005ab2f.) The first relevant EFB copy is EFB copy 3, right before object 423; the second relevant one is EFB copy 4, right before object 440. <details><summary>EFB copy 3 (0005ab11)</summary> ``` BP register BPMEM_EFB_TL EFB Left: 0 EFB Top: 0 BP register BPMEM_EFB_WH EFB Width: 640 EFB Height: 480 BP register BPMEM_EFB_ADDR EFB Target address (32 byte aligned): 0xA44A20 BP register BPMEM_TRIGGER_EFB_COPY Clamping: Top and Bottom Converting from RGB to YUV: No Target pixel format: RGBA8 (6) Gamma correction: 1.0 Half scale: No Vertical scaling: No Clear: Yes Frame to field: Progressive (0) Copy to XFB: No Intensity format: No Automatic color conversion: Yes ``` </details> This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -1,5 +1,7 @@ In addition to the shifting issue, Pokémon Battle Revolution has a black border problem. This happens to be the most obvious with effects that caused shifting, but it occurs in all situations (including the wrist strap screen when starting up the game). This document looks at both the psycho cut fifolog and a rain fifolog, both of which are on [issue 12629](https://bugs.dolphin-emu.org/issues/12629). However, specific object numbers aren't relevant for this issue. # The problem Specifically, the rightmost column and bottommost row of pixels are all black due to a poorly-configured scissor rectangle. The game has the scissor top-left at (342, 342), bottom-right at (980, 820), scissor offset of (342, 342), and the viewport covers (342, 342) to (982, 822). Without going into details on the calculation, this means that the viewport covers pixels from (0, 0) to (639, 479) inclusive, while the scissor covers pixels from (0, 0) to (638, 479) inclusive. This means that pixels with `x=639` or `y=479` never get drawn, and remain black. (Note that this is actually the second scissor the game uses; when it draws shadows, it uses a smaller scissor and viewport which is not affected by the bug.) -
Pokechu22 revised this gist
Dec 18, 2021 . 1 changed file with 1 addition and 1 deletion.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -48,7 +48,7 @@ void GSrender_SetScissor(GSrender *render,uint x,uint y,uint width,uint height) } ``` I have no idea what's going on with the `register0x00000004` stuff other than that it is a `addic.` instruction (add immediate with carry, and then update the status register) on the stack pointer and that `screenWidth`/`scissorHeight` are not set if the result of the add is 0... but there's no sane reason for the stack pointer to be 0, and Ghidra is also doing other weird things here. See [this bug report](https://github.com/NationalSecurityAgency/ghidra/issues/3771). In practice, I think the code is actually something like this: -
Pokechu22 revised this gist
Dec 18, 2021 . 1 changed file with 5 additions and 0 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -0,0 +1,5 @@ Rygar: The Battle of Argus (a port of the PS2 game Rygar: The Legendary Adventure) has a broken bloom effect. In Dolphin, the bloom effect results in the top-left part of the screen being drawn over the entire screen. If the top-left part of the screen is particularly bright, then the whole screen will end up being white. This writeup analyzes the RygarBloom2.dff fifolog on [issue 12763](https://bugs.dolphin-emu.org/issues/12763). The relevant object is 268. -
Pokechu22 revised this gist
Dec 17, 2021 . 1 changed file with 172 additions and 2 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -1,11 +1,181 @@ In addition to the shifting issue, Pokémon Battle Revolution has a black border problem. This happens to be the most obvious with effects that caused shifting, but it occurs in all situations (including the wrist strap screen when starting up the game). # The problem Specifically, the rightmost column and bottommost row of pixels are all black due to a poorly-configured scissor rectangle. The game has the scissor top-left at (342, 342), bottom-right at (980, 820), scissor offset of (342, 342), and the viewport covers (342, 342) to (982, 822). Without going into details on the calculation, this means that the viewport covers pixels from (0, 0) to (639, 479) inclusive, while the scissor covers pixels from (0, 0) to (638, 479) inclusive. This means that pixels with `x=639` or `y=479` never get drawn, and remain black. (Note that this is actually the second scissor the game uses; when it draws shadows, it uses a smaller scissor and viewport which is not affected by the bug.) When the texture is drawn, an indirect texture is used to adjust texture coordinates (the details of which vary depending on the effect the game is rendering, and I go into this more in the other PBR document). The base texture, which is an earlier EFB copy, has clamping enabled. Thus, if the indirect effect causes texture coordinates to go outside of the texture, they end up inside of the texture (instead of being wrapped): `x < 0` becomes `x = 0`, `x > 639` becomes `639`, `y < 0` becomes `0`, `y > 479` becomes `479`. You may see the problem already: `x = 639` and `y = 479` are black due to the scissor, and thus everything beyond them is also black, meaning the indirect effects can bring in large black regions. (For instance, if `x` is already 630, and the indirect texture would result in adding 15 to x, then the result is blackness.) The fix is to adjust the scissor rectangle so that it actually covers the whole screen (by adding 1 to the bottom and right coordinates in this case). Then, the clamping works properly: `x = 639` and `y = 479` both look similar to their neigboring pixels, instead of being completely black. Note that this doesn't magically add additional detail when the indirect texture results in going beyond the edge of the texture; this is easiest to see on the right-hand side of [the rain output image](#file-ZPBR_Rain_out_both-png) where the checkerboard does not continue within the rain effect (but it's a good enough approximation that it doesn't look _wrong_ at first glance either, which is way better than black). Note also that `x = 0` and `y = 0` already worked properly with clamping (though I don't know if this is actually relevant with how the indirect texture is used - TODO: I need to check this still). # The code Here is the relevant code, as generated by Ghidra with some naming but no cleanup (this code is located at `80244a4c` in the NTSC-U version of the game): ```C++ void GSrender_SetScissor(GSrender *render,uint x,uint y,uint width,uint height) { short screenHeight; ushort screenWidth; uint screenSize; render->scissorX = (short)x; render->scissorY = (short)y; render->scissorWidth = (short)width; render->scissorHeight = (short)height; if ((undefined *)register0x00000004 != &DAT_00000006) { screenWidth = render->screenWidth; } if ((undefined *)register0x00000004 != &DAT_00000008) { screenHeight = render->screenHeight; } screenSize = (uint)(ushort)(screenWidth - 1); if (screenSize < x) { x = screenSize; } if ((int)screenSize < (int)((x & 0xffff) + width)) { width = screenSize - x & 0xffff; } screenSize = (uint)(ushort)(screenHeight - 1); if (screenSize < y) { y = screenSize; } if ((int)screenSize < (int)((y & 0xffff) + height)) { height = screenSize - y & 0xffff; } GXSetScissor(x & 0xffff,y & 0xffff,width,height); return; } ``` I have no idea what's going on with the `register0x00000004` stuff other than that it is a `addic.` instruction (add immediate with carry, and then update the status register) on the stack pointer and that `screenWidth`/`scissorHeight` are not set if the result of the add is 0... but there's no sane reason for the stack pointer to be 0, and Ghidra is also doing other weird things here. TODO: link to bug report with simplified test case In practice, I think the code is actually something like this: ```C++ // Calling code already masked x/y/width/height to a 16-bit value, but using ushort in // Ghidra resulted in bad results because it doesn't know that the top 16 bits are all // zero since that masking happened outside of this function. Thus I used uint in Ghidra, // but u16 here. void GSrender_SetScissor(GSrender *render, u16 x, u16 y, u16 width, u16 height) { render->scissorX = x; render->scissorY = y; render->scissorWidth = width; render->scissorHeight = height; u16 maxWidth = render->screenWidth - 1; if (x > maxWidth) { x = maxWidth; } // n.b. I believe the (int) casts appeared in the original because the compiler assumed // x + width will not overflow, and thus doesn't truncate x + width to a 16-bit value. // This would be legal because unsigned int overflow is undefined behavior in C/C++. // It's also preferable behavior because the code's intention isn't to allow extremely // large widths (e.g. x = 0x200, width = 0xff00 would result in x + width = 0x10100 // which truncates to 0x100, which is less than maxWidth, but we'd want width to be // clamped to maxWidth - 0x200 in that case). In any case, this isn't important, // as the game doesn't actually use values that big, and by adding this giant comment // I may have undone the clarity of removing those casts... if (x + width > maxWidth) { width = maxWidth - x; } u16 maxHeight = render->screenHeight - 1; if (y > maxHeight) { y = maxHeight; } if (y + height > maxHeight) { height = screenSize - y; } GXSetScissor(x, y, width, height); } ``` For additional context, `screenWidth` and `screenHeight` are set at `8024439c`. That function is called in the constructor for `GSvideo` at `80243b10`, which is called by the constructor for `GSrender` (at `802359d0`); note that the vtable used by this constructor (which only contains a virtual destructor, I think?) is what gives the names `GSvideo` and `GSrender`. However, this function is rather hard to understand at first glance. It iterates through an array of structs composed of an ID followed by 4 pointers, where that array is located at `80413bf0`; when it finds a struct with a matching ID, it copies data from the appropriate pointer using a memcpy call at `80244560`). The easiest thing to compare it to is libogc's [`VIDEO_GetPreferredMode`](https://github.com/devkitPro/libogc/blob/bd2ea547300fecb11c6258f7ae3b800fdbbab7ae/libogc/video.c#L2820-L2904) but it's not an exact match. On my machine, it ends up copying data from `80424438`, which results in a `screenWidth` of 640 and a `screenHeight` of 480. The one other thing to note is that although the scissor registers are composed of a top-left coordinate and a bottom-right coordinate, and those coordinates are _inclusive_, `GXSetScissor` takes a top-left coordinate and an _exclusive_ width and height. Libogc's [`GX_SetScissor`](https://github.com/devkitPro/libogc/blob/bd2ea547300fecb11c6258f7ae3b800fdbbab7ae/libogc/gx.c#L3934-L3949) ([doc](https://github.com/devkitPro/libogc/blob/bd2ea547300fecb11c6258f7ae3b800fdbbab7ae/gc/ogc/gx.h#L2947-L2967)) is basically the same function. So, `GXSetScissor(0, 0, 640, 480)` would configure the scissor for the whole screen. Now, `GSrender_SetScissor(render, 0, 0, 640, 480)` should do the exact same thing, shouldn't it? Unfortunately, that's not what happens. `maxWidth = renderer->screenWidth - 1 = 639` and `maxHeight = renderer->screenHeight - 1 = 479`. `x <= maxWidth` and `y <= maxHeight`, so `x` and `y` remain 0. But `x + width = 0 + width = 640` is greater than `maxWidth = 639`, so `width` becomes `maxWidth - x = maxWidth - 0 = 639`. The same thing happens to `height`; it becomes `maxHeight - y = 479`. Thus, it ends up calling `GXSetScissor(0, 0, 639, 479)`, and one pixel is no longer included. The easiest fix is to just stop subtracting 1 from `screenWidth` and `screenHeight`. Then, the `GSrender_SetScissor(render, 0, 0, 640, 480)` call results in `x + width <= maxWidth` (as `width == maxWidth`), and the same for `height`, so `GXSetScissor(0, 0, 640, 480)` is called. But now, `GSrender_SetScissor(render, 1000, 1000, 4, 4)` would result in a call of `GXSetScissor(640, 480, 0, 0)` - is that a problem? No, as before that same call would have resulted in `GXSetScissor(639, 479, 0, 0)` which affects on-screen pixels even though the scissor is off-screen. An exclusive width of 0 also seems like it would cause issues, but in practice, if the top-left coordinate is past the bottom-right coordinate, nothing is rendered (this was checked on real hardware). Theoretically, none of this clamping is needed at all, as the scissor functionality works mostly fine with out-of-bounds ranges (as long as the ranges are close enough to in bounds that nothing overflows), but there's no harm in having it. # The patch As determined above, the fix is simply to get rid of the code that subtracts 1 from `screenWidth`/`screenHeight`. This is pretty easy: we just need to replace two instructions: ``` 80244a94: 3908ffff subi r8, r8, 1 80244a9c: 3803ffff subi r0, r3, 1 ``` The first one could be replaced with any kind of NOP, but the second one also moves from `r3` to `r0` in addition to subtracting 1. I think the easiest fix is to change this from a `subi` instruction (which subtracts an immediate value (i.e. a value encoded in the instruction, not a value stored in a register) from the value in one register and stores that result in another register) with a `addi` instruction where the immediate value is set to 0. This is actually trivial, as `subi` is just a special case of `addi` (both of them simply take a 16-bit sign-extended immediate value; if that value has the top bit set, it's negative, and the instruction shows up as `subi`). So we can simply replace the `ffff` with `0000`, to get the following: ``` 80244a94: 39080000 addi r8, r8, 0 80244a9c: 38030000 addi r0, r3, 0 ``` This gives the following Dolphin patch: ``` [OnFrame] $Fix bottom/right pixels being black and black screen effects 0x80244A94:dword:0x39080000 0x80244A9C:dword:0x38030000 ``` which can also be implemented by a Gecko code like this (see [code type documentation](https://wiigeckocodes.github.io/codetypedocumentation.html)): ``` [Gecko] $Fix bottom/right pixels being black and black screen effects [Pokechu22] 04244A94 39080000 04244A9C 38030000 *See https://gist.github.com/Pokechu22/e9fa9037fe21312ff32475638b78ba27 ``` It's worth noting that this code fixes the issue on real hardware — again, this isn't a Dolphin-specific problem, but a game bug. # Patching other versions of the game I only own a USA copy of Pokémon Battle Revolution, so I only have a patch for it. Furthermore, the code in various versions of the game will almost certainly be at slightly different addresses, meaning the same patch won't work on all versions of the game. However, it should be possible to generate patches for other versions without as much trouble by following these steps: 1. Enable Dolphin's debug UI. 2. Select Options → Boot to Pause. 3. Launch Pokémon Battle Revolution. 4. Select Symbols → Generate Symbols From → Signature Database. 5. Select View → Code. 6. In "filter symbols", enter `GXSetScissor`. 7. Select `GXSetScissor` from the symbols list. 8. In the function callers list, there should be 2 entries; one will be `__GXInitGX`, and the other will be something like `zz_80244a4c`. Select the one that is not `__GXInitGX`. 9. Scroll up to the start of the function (different functions are indicated by different background colors). 10. Start scrolling down until you see `subi r8, r8, 1`. There should be a `subi r0, r3, 1` a few instructions below too. 11. For each instruction, right-click and copy the address, and then also copy the hex. The hex should end in `ffff`; changing it to `0000` will solve the issue. A 32-bit memory patch is fine for this. # Note about dolphin Note that when generating images, I used a modified version of Dolphin so that I could use the same fifolog for both images. Here is the change I made: ```patch diff --git a/Source/Core/VideoCommon/BPStructs.cpp b/Source/Core/VideoCommon/BPStructs.cpp index 0fc4ca6785..b0b3940398 100644 --- a/Source/Core/VideoCommon/BPStructs.cpp +++ b/Source/Core/VideoCommon/BPStructs.cpp @@ -128,11 +128,13 @@ static void BPWritten(const BPCmd& bp, int cycles_into_future) // ---------------- // Scissor Control // ---------------- case BPMEM_SCISSORTL: // Scissor Rectable Top, Left case BPMEM_SCISSORBR: // Scissor Rectable Bottom, Right case BPMEM_SCISSOROFFSET: // Scissor Offset + bpmem.scissorBR.x = bpmem.scissorBR.x | 1; + bpmem.scissorBR.y = bpmem.scissorBR.y | 1; SetScissor(); SetViewport(); VertexShaderManager::SetViewportChanged(); GeometryShaderManager::SetViewportChanged(); return; ``` This also fixes the issue, but it's inaccurate for general emulation. However, the software renderer currently works that way due to an implementation mistake (it applied the scissor to 2 by 2 blocks of pixels, rather than to individual pixels). My [scissor pull request](https://github.com/dolphin-emu/dolphin/pull/10251) fixes that. When that issue with the software renderer is fixed, it too will produce the black pixels (unless the patch is applied). Note that there are games where this are needed (shadows inJames Bond - Everything or Nothing can have shadows break, and Samurai Warriors 3 uses it to produce a vignette effect, for instance). -
Pokechu22 revised this gist
Dec 17, 2021 . 1 changed file with 11 additions and 0 deletions.There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -0,0 +1,11 @@ In addition to the shifting issue, Pokémon Battle Revolution has a black border problem. This happens to be the most obvious with effects that caused shifting, but it occurs in all situations (including the wrist strap screen when starting up the game). Specifically, the rightmost column and bottommost row of pixels are all black due to a poorly-configured scissor rectangle. The game has the scissor top-left at (342, 342), bottom-right at (980, 820), scissor offset of (342, 342), and the viewport covers (342, 342) to (982, 822). Without going into details on the calculation, this means that the viewport covers pixels from (0, 0) to (639, 479) inclusive, while the scissor covers pixels from (0, 0) to (638, 479) inclusive. This means that pixels with `x=639` or `y=479` never get drawn, and remain black. (Note that this is actually the second scissor the game uses; when it draws shadows, it uses a smaller scissor and viewport which is not affected by the bug.) When the texture is drawn, an indirect texture is used to adjust texture coordinates (the details of which vary depending on the effect the game is rendering, and I go into this more in the other PBR document). The base texture, which is an earlier EFB copy, has clamping enabled. Thus, if the indirect effect causes texture coordinates to go outside of the texture, they end up inside of the texture (instead of being wrapped): `x < 0` becomes `x = 0`, `x > 639` becomes `639`, `y < 0` becomes `0`, `y > 479` becomes `479`. You may see the problem already: `x = 639` and `y = 479` are black due to the scissor, and thus everything beyond them is also black, meaning the indirect effects can bring in large black regions. (For instance, if `x` is already 630, and the indirect texture would result in adding 15 to x, then the result is blackness.) The fix is to adjust the scissor rectangle so that it actually covers the whole screen (by adding 1 to the bottom and right coordinates in this case). Then, the clamping works properly: `x = 639` and `y = 479` both look similar to their neigboring pixels, instead of being completely black. Note that this doesn't magically add additional detail when the indirect texture results in going beyond the edge of the texture; this is easiest to see on the right-hand side of [the rain output image](#file-ZPBR_Rain_out_both-png) where the checkerboard does not continue within the rain effect (but it's a good enough approximation that it doesn't look _wrong_ at first glance either, which is way better than black). Note also that `x = 0` and `y = 0` already worked properly with clamping (though I don't know if this is actually relevant with how the indirect texture is used - TODO: I need to check this still). TODO: Write up WHY the game was not working right (80244a4c and 8024439c, note the latter's memcpy) Note that when generating images, I used a modified version of Dolphin so that I could use the same fifolog for both images. -
Pokechu22 revised this gist
Dec 17, 2021 . 14 changed files with 0 additions and 0 deletions.There are no files selected for viewing
LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed.LoadingSorry, something went wrong. Reload?Sorry, we cannot display this file.Sorry, this file is invalid so it cannot be displayed. -
Pokechu22 created this gist
Dec 17, 2021 .There are no files selected for viewing
Empty file.Empty file.Empty file.