Conversation
JaceCear
left a comment
JaceCear
left a comment
There was a problem hiding this comment.
I have not looked at the rendering code.
Otherwise, please adjust/discuss! :)
| } else { | ||
| track->repeatCount = 0; | ||
| track->cmdPtr += sizeof(u8) + sizeof(u8 *); | ||
| track->cmdPtr = track->cmdPtr + sizeof(u8 *); |
There was a problem hiding this comment.
Why is this changed from += 5(32bit) / 9(64bit) to 8?
And if this is indeed correct behavior, it can be downsized to += sizeof(u8 *). If it is correct.
There was a problem hiding this comment.
Just had a look at the original assembly.
The way it was before is correct (+= 5 in the original), so please revert it back!
There was a problem hiding this comment.
Was a mistake, oops
| #if PLATFORM_WIN32 && !PLATFORM_SDL | ||
| #define RENDERER_SOFTWARE 0 | ||
| #define RENDERER_OPENGL 1 | ||
| #define RENDERER_SOFTWARE_FAST 2 |
There was a problem hiding this comment.
I'm not a fan of having 2 software renderers.
Especially considering it's to-be-removed anyway, adding another label here is extra complexity imo.
We can replace the old rendering with the faster one (leaving comments intact if possible) and if bugs occur we'll figure out, why.
My intention with these #defines was specifically to list different backends here.
| #define PSP_SCREEN_W 480 | ||
| #define PSP_SCREEN_H 272 |
There was a problem hiding this comment.
Are those actually nowhere defined in the PSP SDK?
If they are, remove these and use the SDK ones instead!
There was a problem hiding this comment.
| // Every GBA frame we process the SDL events and render the number of times | ||
| // SDL requires us to for vsync. When we need another frame we break out of | ||
| // the loop via a return |
There was a problem hiding this comment.
No reason to be less verbose in the comment, I'd prefer keeping the old one.
| #define BTN_TRIANGLE 12 | ||
| #define BTN_CIRCLE 13 | ||
| #define BTN_CROSS 14 | ||
| #define BTN_SQUARE 15 |
There was a problem hiding this comment.
As a recommendation, I think calling them BTN_FACE_LEFT|TOP|RIGHT|BOTTOM would make code using face buttons more portable.
Here I'd also want you to check, whether the SDK defines these or not, and use those defines if they do.
There was a problem hiding this comment.
I had a look but it doesn't seem to (if we use the SDL interface)
| // Handle horizontal and vertical tile flipping | ||
| if (entry & (1 << 10)) | ||
| tileX = (TILE_WIDTH - 1) - tileX; // H-flip | ||
| tx = 7 - tx; |
There was a problem hiding this comment.
Using magic numbers instead of TILE_WIDTH is less explicit.
| uint32_t tileDataOffset = tileNum << 5; | ||
| uint32_t pixelByteOffset = (ty << 2) + (tx >> 1); | ||
| uint8_t pixelPair = bgtiles[tileDataOffset + pixelByteOffset]; | ||
|
|
||
| uint8_t pixel; | ||
| if (tileX & 1) { | ||
| pixel = pixelPair >> 4; | ||
| } else { | ||
| pixel = pixelPair & 0xF; | ||
| } | ||
| uint8_t pixel = (tx & 1) ? (pixelPair >> 4) : (pixelPair & 0xF); | ||
|
|
||
| if (pixel != 0) { | ||
| line[x] = pal[16 * paletteNum + pixel] | 0x8000; | ||
| line[x] = pal[(paletteNum << 4) + pixel] | 0x8000; | ||
| } | ||
| } else { // 8 bits per pixel | ||
| uint32_t tileDataOffset = tileNum * TILE_SIZE_8BPP; | ||
| uint32_t pixelByteOffset = tileY * TILE_WIDTH + tileX; | ||
| uint32_t tileDataOffset = tileNum << 6; | ||
| uint32_t pixelByteOffset = (ty << 3) + tx; |
There was a problem hiding this comment.
Does the compiler actually not optimize these multiplies to bitshifts?
Even agbcc does, and that's an OLD compiler...
| @@ -872,7 +872,7 @@ NONMATCH("asm/non_matching/engine/sub_80039E4.inc", bool32 sub_80039E4(void)) | |||
| return TRUE; | |||
| #endif | |||
|
|
|||
| #if (RENDERER == RENDERER_SOFTWARE) | |||
| #if (RENDERER != RENDERER_OPENGL) | |||
There was a problem hiding this comment.
If we keep RENDERER_SOFTWARE_FAST (which as I said I would not prefer), please add that as extra case, instead of != RENDERER_OPENGL, because it's more explicit!
Otherwise change it back to original.
| @@ -939,7 +939,7 @@ bool32 ProcessVramGraphicsCopyQueue(void) | |||
| if ((graphics->src != 0) && (graphics->dest != 0)) | |||
| #endif | |||
| { | |||
| #if (RENDERER == RENDERER_SOFTWARE) | |||
| #if (RENDERER != RENDERER_OPENGL) | |||
There was a problem hiding this comment.
If we keep RENDERER_SOFTWARE_FAST (which as I said I would not prefer), please add that as extra case, instead of != RENDERER_OPENGL, because it's more explicit!
Otherwise change it back to original!
3 participants
No description provided.