clean up doom_get_framebuffer code #18
Conversation
PureDOOM.h
Outdated
| int kpal = screen_buffer[i] * 3; | ||
| final_screen_buffer[k + 0] = screen_palette[kpal + 0]; | ||
| final_screen_buffer[k + 1] = screen_palette[kpal + 1]; | ||
| final_screen_buffer[k + 2] = screen_palette[kpal + 2]; | ||
|
|
||
| if (channels == 4) |
There was a problem hiding this comment.
I repeat code because this line creates branching in a loop that should be as tight as possible.
Altho branch prediction will make this fast anyway, I rather keep every channel code separated. It's not like it's a lot of code anyway, to guarantee tightess loop.
There was a problem hiding this comment.
Sure, I'm not too worried about the speed for that.
-
the compiler might optimize it out anyway.
-
your library works with a smaller bitmap, so that part of processing can be slower.
I think it should stay small because it allows the user to change the size themselves and/or modify the smaller bitmap if they so choose. This comment is in response to one of your todo items.
Looking back at this code again, if we care about speed maybe it would be faster to use something like memcpy?
There was a problem hiding this comment.
This could also be fixed by using a macro I'd think.
Eg. DOOM_SUPPORT_ALPHA or something
Then the logic is done in preprocessing only once.
README.md
Outdated
| @@ -17,6 +17,7 @@ See end of file for license information. | |||
| - Menu option to disable mouse move Forward/Backward | |||
| - Crosshair option | |||
| - Always-Run option | |||
| - Print message when discovered a secret | |||
src/DOOM/DOOM.c
Outdated
| { | ||
| return 0; | ||
| } | ||
| return 0; |
|
This needs to be rebased and squashed |
|
Sure, I would like to work out the issues first. |
src/DOOM/DOOM.c
Outdated
| else if (channels == 4) | ||
| { | ||
| for (i = 0, len = SCREENWIDTH * SCREENHEIGHT; i < len; ++i) | ||
| { | ||
| int k = i * 4; | ||
| int kpal = screen_buffer[i] * 3; | ||
| final_screen_buffer[k + 0] = screen_palette[kpal + 0]; | ||
| final_screen_buffer[k + 1] = screen_palette[kpal + 1]; | ||
| final_screen_buffer[k + 2] = screen_palette[kpal + 2]; | ||
| final_screen_buffer[k + 3] = 255; | ||
| } | ||
| return final_screen_buffer; | ||
| } |
There was a problem hiding this comment.
Put back the duplicated code. Good thinking though. But it's ok to duplicate code that have difference purpose, especially small block like this. Some CPUs will have the branch prediction, but some others like on micro controllers, might not. Reminder this is to be run on anything. I prefer keeping it separate and branch-less if it doesnt have to.
The 2 other changes are good. (The return 0 and the readme fix)
Rebase & Squash
There was a problem hiding this comment.
That was the whole point of this pull request
There was a problem hiding this comment.
You're probably right when it comes to the CPU. But also I'd think this is something that would be caught by the compiler first.
|
My bad, it should be doom_memset not memset. Honestly, I don't know why you're using functions for this instead of macros It would be so much easier for everyone. eg. #ifndef DOOM_MALLOC |
src/DOOM/DOOM.c
Outdated
| @@ -556,6 +556,7 @@ void doom_init(int argc, char** argv, int flags) | |||
|
|
|||
| screen_buffer = doom_malloc(SCREENWIDTH * SCREENHEIGHT); | |||
| final_screen_buffer = doom_malloc(SCREENWIDTH * SCREENHEIGHT * 4); | |||
| doom_memset(final_screen_buffer, 0x000000FF, SCREENWIDTH * SCREENHEIGHT * 4); | |||
There was a problem hiding this comment.
Good catch, an oversight on my part.
You repeat code if there are 3 or 4 channels, this can be easily reduced. Also, you don't need the redundant else because it would've returned by then anyway.