SDLPoP; David's open-source port of PoP

[Falcury]

I think I fixed the hitpoint bug:
* In set_chtab_palette: (used for coloring the guards)

Code: Select all

- scolors[i].a = 0;
+ scolors[i].a = SDL_ALPHA_OPAQUE;

* In method_6_blit_img_to_scr:

Code: Select all

(in the else branch)
-        //SDL_SetSurfaceAlphaMod(colored_image, 0);
+        SDL_SetSurfaceAlphaMod(colored_image, 255);

Some more bugs that I found:
* shift,ctrl,alt,num,caps,scroll,... un-pause the game.
* If the game is paused, the screen is not redrawn (for example when I drag the window off-screen and back).
* Name entry does not work correctly. (hall of fame)

Nice! I have spent some time trying to fix the other bugs you mentioned;
- text entry should work correctly now
- the game now updates the screen while paused and modifier keys should now leave the game paused
- the program does not display the console window anymore on Windows.
I've pushed the changes to GitHub as well.

[Norbert]

I haven't read any of the new code, but it's cool that you folks looked into further improving SDLPoP.
It will probably take some time for David to process everything.

[Falcury]

Hopefully I haven't caused more regressions - I really shouldn't leave the code worse off than I found it. Learning as I go...

[David]

* If the game is paused and I press Ctrl-A,Ctrl-Q,Shift-L,Shift-T,Shift-W,Shift-I,... then that key is *ignored*. (Except that the game is unpaused.)
But h,j,u,n,+,-,k are not ignored.

A possible solution:

Code: Select all

In process_key(): (seg000.c)
And in key_test_quit(): (seg009.c)
- if (!control_ctrl) break;
+ if (!key_states[SDL_SCANCODE_LCTRL] && !key_states[SDL_SCANCODE_RCTRL]) break;
And similarly for shift.

Another solution:
Store event.key.keysym.mod somewhere when processing a SDL_KEYDOWN event.
And use that instead of control_ctrl/shift when checking for key combinations.
For example: control_ctrl -> (last_key_modifier & KMOD_CTRL)

* In sound_timer = NULL and the like, replace NULL with 0.
In SDL1, SDL_TimerID was a pointer. In SDL2, it is an int.
(The compiler gives a warning: assignment makes integer from pointer without a cast.)

[Falcury]

OK, more fixes:
- Fixed hotkeys not working when the game is paused (I used your first solution in order to avoid adding new global variables)
- Fixed SDL_TimerID implicit casts

I tried renaming a couple of things to improve readability:
- Added enum for sound IDs
- Added enum for char_type.sword (sword_0_sheathed and sword_2_drawn)
- Renamed chtab.pointers to chtab.images

I also found another issue: rooms are not drawn properly anymore after a loose tile in the rightmost column has dropped out of the room. Not sure why this is...

[kees]

- Added enum for sound IDs

I have this in my branch too. Can someone merge my pull request? It's got a bunch of fixes like this that should really help find real warnings/errors when compiling.

[Falcury]

I intentionally left that unfixed because I knew your solution would be merged in I meant an enum for the IDs of individual sounds; this way, an informative name can be passed to play_sound() instead of plain numbers.

[David]

I also found another issue: rooms are not drawn properly anymore after a loose tile in the rightmost column has dropped out of the room. Not sure why this is...

(I tried this on level 10, room 2.)
Something turns blind mode on. (It can be turned off with Shift-B.)
This could be caused by an index overflow. (*)

(*) The original game has a similar bug: viewtopic.php?p=16082#p16082
But there the checkpoint is activated (not the blind mode), and the loose floor has to be in the ceiling of the room.

This does not happen in the SDL1 version.
Interestingly, the bug disappears if I recompile your v1.14c.
Maybe your compiler arranges data differently?

Can someone merge my pull request?

Erm, yes, I should merge the requests of both of you.
Is it possible to merge both correctly?

And then I could also add my own changes.
For example, the Makefiles and the *.dev project files need updating.
And some things could be added to Readme.txt. (GitHub address, contributors, compiling with SDL2.)

[David]

I merged kees's changed with the web GUI.
Then I tried to merge Falcury's, but it said "We can’t automatically merge this pull request.".
So I tried to merge it from the command line.
I could merge it, I even added some changes, but I could not commit it.
(GIT kept "finding excuses", like "Everything up-to-date" or "Changes not staged for commit:")
Finally I accidentally destroyed my changes while desperately trying to commit.

(Maybe I should *learn* GIT before using it... )

[Norbert]

(GIT kept "finding excuses", like "Everything up-to-date" or "Changes not staged for commit:")
Finally I accidentally destroyed my changes while desperately trying to commit.

(Maybe I should *learn* GIT before using it... )

Quoting icculus (audio source): "I think Git is an awful piece of software. [...] Every time I use Git I end up destroying my local clone of the repository just by accident."

[Falcury]

I'm currently using SourceTree to visually manage my local changes, but even so I managed to do the same thing a couple of days ago...

And apparently the effects of an array index being out of bounds are indeed unpredictable... Fix for the loose tile bug:

Code: Select all

in set_redraw2(): (seg007.c)
if (tilepos < 0) {
	// trying to draw a mob at a negative tilepos, in the range -1 .. -10
	// used e.g. when the kid is climbing up to the room above
	// however, loose tiles falling out of the room also end up with a negative tilepos {-2 .. -11} !
	tilepos = (-tilepos) - 1;
	if (tilepos > 9) tilepos = 9; // prevent array index out of bounds!
	redraw_frames_above[tilepos] = frames;
}

[kees]

I intentionally left that unfixed because I knew your solution would be merged in I meant an enum for the IDs of individual sounds; this way, an informative name can be passed to play_sound() instead of plain numbers.

Ah! Okay, cool.

GIT kept "finding excuses", like "Everything up-to-date" or "Changes not staged for commit:"

Yeah, git can be weird, and I find some aspects of the github GUI confusing.

[Falcury]

Here is a new version of the SDL2 port. I have tried to clean up some of the ugly workarounds I had initially added to get SDL2 to work. The game also runs more efficiently now.

My 'quick fix' to make blitting work was to convert all 8-bit images to 32-bit equivalents immediately before the program wanted to draw them. This of course created more problems than it solved (probably) and added cumbersome complexity. So, I have tried to revert this as much as possible to the way it worked with SDL1;
- I removed convert_surface_to_onscreen_format(); 8 bpp images are now properly blitted directly again
- The onscreen and offscreen surfaces are now 24 bpp again instead of 32. Unfortunately, I cannot get this surface to the screen correctly, so for now it is still converted to 32 bpp every time there is a screen update (which is at least less horrible than the way it used to be).

One strange side effect is that I had to change the argument order (b,g,r instead of r,g,b) in two calls to SDL_MapRGB(). Colors are normal using this hack but I don't understand why it became necessary:

Code: Select all

in method_5_rect() and set_bg_attr(): (seg009.c)
// @Hack: byte order (rgb) is reversed (otherwise the color is wrong) - why doesn't this work as expected?
    uint32_t rgb_color = SDL_MapRGB(onscreen_surface_->format, palette_color.b<<2, palette_color.g<<2, palette_color.r<<2);

To improve the game's performance, I have reduced the number of times the screen is updated (it seems this was a bottleneck on my older laptop). Previously, the game would update the screen almost every time anything got drawn, unnecessarily. This is what I've changed:
- method_3_blit_mono() no longer requests screen updates; text should now display instantly instead of letter by letter. A screen update is now manually invoked in show_text() and in a few other places.
- I have added a global variable screen_updates_suspended which is checked by request_screen_update() every time a screen update is requested (renamed from screen_update()). The heavy drawing routines such as draw_game_frame(), redraw_screen() etc now have the power to temporarily suspend screen updates so that they only need to request one update after they are done drawing everything.

[David]

Quoting icculus (audio source): "I think Git is an awful piece of software. [...] Every time I use Git I end up destroying my local clone of the repository just by accident."

Oh... It's good to know I'm not alone.

text should now display instantly instead of letter by letter

Yes, that caused a noticeable pause in full-screen mode.

they only need to request one update after they are done drawing everything.

It should be enough to update the screen in the main loop(s), like play_level_2(), proc_cutscene_frame().
And also in draw_image_2().
(I didn't test this!)

SDL_MapRGB(onscreen_surface_->format, palette_color.b<<2, palette_color.g<<2, palette_color.r<<2);

Should that be current_target_surface->format instead?

[Falcury]

Should that be current_target_surface->format instead?

Probably, yes. Although it shouldn't make a difference as the on- and offscreen surfaces have the same pixel format?