Refactor Code of TextureConverter #3844
Conversation
|
Can one of the admins verify this patch? |
|
Assuming this refactor cleans up the code in an effective way -- it does get rid of a bunch of seemingly (from my perspective) valuable comments and documentation. (A lot of the newly introduced comments are just stating the obvious as well) |
I can try to add the comments back, and for the newly introduced comments, i tried to describe the functions, so if someone hovers over it, it gives some details |
There was a problem hiding this comment.
Thanks for taking a look at this!
I don't think this refactoring is correct for at least PSM4T.
In the future, if you are going to make a refactoring like this, please try to either include tests or some other proof that your changes are correct. Or make the changes so clear that a reviewer can be convinced easily.
| }; | ||
|
|
||
| // Pixel processing function | ||
| auto process_pixel = [&](u32 x, u32 y, u32 addr, int clut_psm) -> u32 { |
There was a problem hiding this comment.
x and y are not used in this lambda
There was a problem hiding this comment.
yea i refactored everything again
| // Main loop | ||
| for (u32 y = 0; y < h; y++) { | ||
| for (u32 x = 0; x < w; x++) { | ||
| u32 addr = 0; |
There was a problem hiding this comment.
This addr variable is used incorrectly - in some cases it is treated as an address into the index texture, and others it is a value from the index texture (which is an encoding of an address into the clut)
There was a problem hiding this comment.
I tried to initialize address depending on the old code. Now it should be right. Hopefully
| for (u32 x = 0; x < w; x++) { | ||
| u32 addr = 0; | ||
| if (psm == int(PSM::PSMT8)) { | ||
| addr = psmt8_addr(x, y, read_width) + vram_addr * 256; |
There was a problem hiding this comment.
Here, it is an address into the index texture.
| value = (addr4 & 1) ? (value >> 4) : (value & 0x0F); | ||
| addr = value; |
There was a problem hiding this comment.
Here, it is set to a value from the index texture, which contains an encoded address into the clut. (notice the u8 value = *(u8*)(m_vram.data() + addr4 / 2); that's reading the texture).
There was a problem hiding this comment.
Jep that was wrong. Fixed it
|
|
||
| // Pixel processing function | ||
| auto process_pixel = [&](u32 x, u32 y, u32 addr, int clut_psm) -> u32 { | ||
| u8 value = *(u8*)(m_vram.data() + addr); |
There was a problem hiding this comment.
This method of reading doesn't handle 4-bit index textures correctly (PSMT4)
| u32 clut_chunk = value / 16; | ||
| u32 off_in_chunk = value % 16; | ||
| u8 clx = (clut_chunk & 1) ? 8 : 0; | ||
| u8 cly = (clut_chunk >> 1) * 2; | ||
| if (off_in_chunk >= 8) { | ||
| off_in_chunk -= 8; | ||
| cly++; |
There was a problem hiding this comment.
This scrambling is only valid for PSMT8 index textures and doesn't work for PSMT4 (see deleted comments about consulting GS manual 2.7.3 CLUT Storage Mode, IDTEX8 in CSM1 mode.)
| // Pixel processing function | ||
| auto process_pixel = [&](u32 x, u32 y, u32 addr, int clut_psm) -> u32 { |
There was a problem hiding this comment.
This comment is not very meaningful, and it's not intuitive to figure out what this function does. Is addr an address into the clut or the index texture? What does it return? Something like
auto lookup_rgba_from_clut = [&](u32 index_texture_value, int clut_psm) -> u32
may be more clear.
Or, since this lambda is only used one, you could avoid creating this entirely.
There was a problem hiding this comment.
The lambda has been renamed, but since its called twice i avoided removing it
| * @param height (Optional) Texture height in texels. Used if `size_vram_words` is not provided. | ||
| * @param size_vram_words (Optional) Texture size in VRAM words. Assumes a width of 128 texels. | ||
| */ | ||
| void upload(const u8* data, u32 dest, u32 width = 0, u32 height = 0, u32 size_vram_words = 0); |
There was a problem hiding this comment.
I prefer to use default arguments only when the defaults are valid, and prefer multiple implementations over tricky use of defaults.
For example, converter.upload(data, dest); will fail the validation, but looks like a valid call.
There was a problem hiding this comment.
the test-case with converter.upload(data,dest) is validated by:
if ((size_vram_words == 0 && (width == 0 || height == 0)) || data == nullptr) {
throw std::invalid_argument(
"Invalid parameters: Provide either size_vram_words or width and height, and ensure data "
"is not null.");
}
I have Refactored the code a bit:
For upload, combining both and adding default values reduces duplicate Code, also when is the size_vram_words given? I did not find any usecases