SDLPoP; David's open-source port of PoP

Open-source port of PoP that runs natively on Windows, Linux, etc.
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: SDLPoP; David's open-source port of PoP

Post by Norbert »

Norbert wrote: October 30th, 2022, 9:47 amI'll try to find time in the coming weeks/months to add teleports to apoplexy's "native tiles screen" (which is accessible via Ctrl+click and "x").
Done; released apoplexy 3.16.

Hopefully, "teleports" will shortly be merged into master, for an erelong v1.23 release.
I can put together a brief teleports how-to video, similar to the colored torch flames how-to video.
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP

Post by David »

Norbert wrote: November 16th, 2022, 10:29 pm Hopefully, "teleports" will shortly be merged into master, for an erelong v1.23 release.
With this change, is it ready for merging?
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: SDLPoP; David's open-source port of PoP

Post by Norbert »

David wrote: November 19th, 2022, 12:36 pm
Norbert wrote: November 16th, 2022, 10:29 pm Hopefully, "teleports" will shortly be merged into master, for an erelong v1.23 release.
With this change, is it ready for merging?
I think so. I've tested the most recent code, and it works.

Unrelated, screenshot.c throws 2 warnings.
1. line 41, snprintf: output may be truncated writing 12 bytes into a region of size between 1 and 256
2. line 32, strncpy: specified bound 256 equals destination size
Maybe not important, but just in case.
Norbert wrote: November 16th, 2022, 10:29 pmI can put together a brief teleports how-to video, similar to the colored torch flames how-to video.
I've finished making this video, so it's ready to be added to the PoPModding YouTube channel whenever the new SDLPoP release arrives.
FluffyQuack
Vizier
Vizier
Posts: 80
Joined: June 6th, 2004, 7:05 pm

Re: SDLPoP; David's open-source port of PoP

Post by FluffyQuack »

David wrote: November 12th, 2022, 12:41 pmI have added similar functionality on the branch "redefine_keyboard2".
It adds controls as a separate menu and you can change any key without having to change all the others.

It looks like this:
redefine_keys.png
Oh, then I don't need to work on my branch for that anymore. You've already gotten more progress on it than me.

It would be nice to have the joystick rebindable too. I tried the Xbox ports of POP1/POP2 recently and I like what they did with the controls there:
D-pad \ left stick = Movement
LT\RT = Grab
X = Attack
A = Jump (aka up)
B = Crouch (aka down)
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP

Post by David »

Norbert wrote: November 19th, 2022, 4:26 pm
David wrote: November 19th, 2022, 12:36 pm With this change, is it ready for merging?
I think so. I've tested the most recent code, and it works.
Done: https://github.com/NagyD/SDLPoP/commit/ ... 12bca86752
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP

Post by David »

Norbert wrote: November 19th, 2022, 4:26 pm Unrelated, screenshot.c throws 2 warnings.
1. line 41, snprintf: output may be truncated writing 12 bytes into a region of size between 1 and 256
2. line 32, strncpy: specified bound 256 equals destination size
Maybe not important, but just in case.

1. is this line:

Code: Select all

teleports:
		snprintf(screenshot_filename, sizeof(screenshot_filename), "%s/screenshot_%03d.png", screenshots_folder, screenshot_index);
master:
		snprintf_check(screenshot_filename, sizeof(screenshot_filename), "%s/screenshot_%03d.png", screenshots_folder, screenshot_index);
I'm not sure what is the compiler trying to say here.
"12 bytes" may be the size of the "/screenshot_" part, between the two placeholders.
And certainly, that might be truncated if screenshots_folder is long enough.

2. is this line:

Code: Select all

teleports:
	strncpy(screenshots_folder, locate_file("screenshots"), sizeof(screenshots_folder));
master:
	snprintf_check(screenshots_folder, sizeof(screenshots_folder), "%s", locate_file("screenshots"));
Again, I'm not sure what is the compiler trying to say here.
Isn't the whole point of the bound that it's equal to the size of the destination buffer?

Maybe it's referring to the fact that strncpy doesn't null-terminate the destination if the source is longer than the bound?


In any case, both calls have been changed to snprintf_check() in this commit: https://github.com/NagyD/SDLPoP/commit/ ... 084c0cc64a
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP

Post by David »

FluffyQuack wrote: November 22nd, 2022, 6:13 pm It would be nice to have the joystick rebindable too.
I don't have a joystick or gamepad, so that has to be done by someone else...
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: SDLPoP; David's open-source port of PoP

Post by Norbert »

Norbert wrote: January 6th, 2021, 3:37 pm There are inconsistencies in doc/Readme.txt when it comes to how keyboard shortcuts are presented to the reader.

Example:
- - - - -
* Ctrl-S: sound on/off
* mute -- Start the game with sound off. (You can still enable sound with Ctrl+S.)
- - - - -

Another (double) example:
- - - - -
* Shift-F (while viewing a replay): skip forward to the next level
While viewing a replay, you can press F to skip forward to the next room, or Shift+F to skip to the next level.
* shift+left/right: careful step
- - - - -

Yet another example:
- - - - -
* r: resurrect kid
* Ctrl-R: return to intro
- - - - -

Personally, I always do e.g. Ctrl+r (because plus indicates addition, and a capital r (R) would indicate Ctrl+Shift+r).
But I'm not sure whether that is also the best solution in this particular case/context.
(If you make any changes related to this, I suggest not including them with the upcoming release. To give yourself enough time to look at it without any added pressure.)
I know you already did this, but I just ran into the casing issue again.
As an example, the documentation says, "T: Toggle display of timer".
To me, "T" is a capital t, which makes me do Shift+t, which increases hit points.
[Edit: Maybe it is just me and my programmer mind. That nobody else is confused by the documentation.]
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: SDLPoP; David's open-source port of PoP

Post by Norbert »

I see you've documented teleports in doc/tiles.md. Maybe also mention USE_COLORED_TORCHES for torch (0x13) and torch with debris (0x1E): 0x01-0x3F 6-bit RGB.

Maybe also rewrite this portion

Code: Select all

Since version 1.16, there is support for fake tiles, for example walls that the prince can go through. The Apoplexy level editor supports these additional tiles since v3.0. (Just don't overuse them, please!)

See doc/tiles.md for documentation on fake tiles.
to provide more generalized information, i.e. not about specifically the fake tiles.

Similarly, doc/tiles.md currently states "That page includes the combinations supported by the original game and also the fake tiles." which could be updated to reflect the wiki page (now) (also) lists all native tiles.
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP

Post by David »

Norbert wrote: December 22nd, 2022, 4:19 pm Maybe also mention USE_COLORED_TORCHES for torch (0x13) and torch with debris (0x1E): 0x01-0x3F 6-bit RGB.

Maybe also rewrite this portion
[...]
to provide more generalized information, i.e. not about specifically the fake tiles.

Similarly, doc/tiles.md currently states "That page includes the combinations supported by the original game and also the fake tiles." which could be updated to reflect the wiki page (now) (also) lists all native tiles.
Done: https://github.com/NagyD/SDLPoP/commit/ ... f45dc4f657
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP (pre-release)

Post by David »

Falcury wrote: May 24th, 2015, 9:10 pm - Guard palettes do not seem to get set correctly on Mac OS X, causing a crash (I think the guard images are not loaded as 8-bit for some reason - I haven't thoroughly researched this)
It looks like the newest SDL_image (2.6.2) loads 8-bit (paletted, indexed) images as RGB(A) even on Windows!

An older version, SDL_image 2.0.5 loads 8-bit images as 8-bit images.

I noticed this when I tried to make maps for Christmas of Persia with SDLPoP.
The GUARD.DAT of that mod is missing an image, which causes SDLPoP to load data/GUARD/res776.png instead.
If I use SDL_image 2.6.2, the resulting surface has no palette, thus current_palette->ncolors crashes.
I fixed the crash here: https://github.com/NagyD/SDLPoP/commit/ ... 51f993f598

A quick search shows that it's a known problem: https://github.com/libsdl-org/SDL_image/issues/298
They write that this happens because SDL_image switched from libpng (and libjpg) to stb_image.
The release notes show they switched in 2.6.0.

This same change might be behind a bug which I recently noticed with teleports:
With SDL_image 2.6.2, the teleport images have dark blue/red backgrounds.
Those are the actual colors "behind" the fully transparent parts of the PNGs.
teleport background bug.png
teleport background bug.png (4.68 KiB) Viewed 5503 times
Here is the fix: https://github.com/NagyD/SDLPoP/commit/ ... 2f1eae7e68
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: SDLPoP; David's open-source port of PoP

Post by Norbert »

Maybe post a release candidate, as you did with 1.22? I sort-of assumed you were waiting for Christmas or New Year to release the next version, but you've been holding off so far. My assumption is also you always have the build environment ready, so it shouldn't be the amount of work that's the hurdle holding you back. Oh, don't forget checklist.txt, you created, which includes a line about updating the copyright years.
User avatar
Norbert
The Prince of Persia
The Prince of Persia
Posts: 5743
Joined: April 9th, 2009, 10:58 pm

Re: SDLPoP; David's open-source port of PoP

Post by Norbert »

Couple of random suggestions.

- In the "SETTINGS" submenu, after choosing "BACK", still have "SETTINGS" (re)selected.

- In menu.c, change "QUICKSAVE" and "QUICKLOAD" to respectively "QUICKSAVE (F6)" and "QUICKLOAD (F9)".

- If the above suggestion is accepted, in seg000.c, change
-----
#ifdef USE_QUICKSAVE
"To quick save/load, press F6/F9 in-game.\n"
"\n"
#endif
-----
to
-----
"In-game, the Esc key opens a settings menu.\n"
"\n"
-----
Or something similar, e.g.
"In-game, Esc and Backspace open a menu.\n"
"In-game, Esc opens a settings/quicksave menu.\n"
etc.
(The crux of the idea being that if the menu already includes quicksaving entries and explains its shortcuts, the splash screen is more informative if it mentions the menu.)

- For consistency, in menu.c, edit "Fix quick save in feather mode" to say quicksave (without the space).

- Feedback from gcc 11.3.0. In options.c, function load_dos_exe_modifications(), opcode - interestingly, for the "else if" - may be used uninitialized.

- Feedback from "cppcheck --force --language=c":

Code: Select all

lighting.c:42:96: error: Signed integer overflow for expression '0xFF<<24'. [integerOverflow]
 screen_overlay = SDL_CreateRGBSurface(0, 320, 192, 32, 0xFF << 0, 0xFF << 8, 0xFF << 16, 0xFF << 24);
                                                                                               ^
replay.c:212:4: error: Common realloc mistake: 'replay_list' nulled but not freed upon failure [memleakOnRealloc]
   replay_list = realloc( replay_list, max_replay_files * sizeof( replay_info_type ) );
   ^
screenshot.c:581:113: error: Signed integer overflow for expression '0xFF<<24'. [integerOverflow]
 SDL_Surface* map_surface = SDL_CreateRGBSurface(0, image_width, image_height, 32, 0xFF, 0xFF<<8, 0xFF<<16, 0xFF<<24);
                                                                                                                ^
seg000.c:2083:6: error: Used file that is not opened. [useClosedFile]
 if (fwrite(&rem_tick, 1, 2, handle) != 2) goto loc_1D9B;
     ^
seg000.c:2084:6: error: Used file that is not opened. [useClosedFile]
 if (fwrite(&current_level, 1, 2, handle) != 2) goto loc_1D9B;
     ^
seg000.c:2085:6: error: Used file that is not opened. [useClosedFile]
 if (fwrite(&hitp_beg_lev, 1, 2, handle) != 2) goto loc_1D9B;
     ^
seg009.c:2435:93: error: Signed integer overflow for expression '0xFF<<24'. [integerOverflow]
  overlay_surface = SDL_CreateRGBSurface(0, 320, 200, 32, 0xFF, 0xFF << 8, 0xFF << 16, 0xFF << 24) ;
                                                                                            ^
seg009.c:921:93: error: Signed integer overflow for expression '0xFF<<24'. [integerOverflow]
 return SDL_CreateRGBSurface(0, rect->right, rect->bottom, 32, 0xFF, 0xFF<<8, 0xFF<<16, 0xFF<<24);
                                                                                            ^
seg009.c:1733:139: error: Signed integer overflow for expression '0xFF<<24'. [integerOverflow]
 SDL_Surface* peel_surface = SDL_CreateRGBSurface(0, rect->right - rect->left, rect->bottom - rect->top, 32, 0xFF, 0xFF<<8, 0xFF<<16, 0xFF<<24);
                                                                                                                                          ^
stb_vorbis.c:5411:0: error: failed to expand 'realloc', Wrong number of parameters for macro 'realloc'. [preprocessorErrorDirective]
         data2 = (short *) realloc(data, total * sizeof(*data));
^
- SDLPoP's Makefile uses -std=gnu99, while apoplexy's Makefile uses -std=c99. My thinking for picking c99 was that gnu99 may decrease cross-platform compatibility. Simply because "gnu" is probably "GNU", which is mostly the Unix-like arena. But I'm not sure if my reasoning was correct. It says here that "-std=gnu99 (for C99 with GNU extensions)", which would make c99 a subset of gnu99, and thus c99 might be more widely supported. SDLPoP's Makefile seems to work fine with -std=c99, so you may consider looking into using that, to make it less likely that coders will ever unknowingly use GNU extensions.
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP

Post by David »

Norbert wrote: January 3rd, 2023, 11:50 am Oh, don't forget checklist.txt, you created, which includes a line about updating the copyright years.
Done: https://github.com/NagyD/SDLPoP/commit/ ... 6640e4f66a
Norbert wrote: January 4th, 2023, 1:58 pm - In the "SETTINGS" submenu, after choosing "BACK", still have "SETTINGS" (re)selected.
Done: https://github.com/NagyD/SDLPoP/commit/ ... 745084887c
Norbert wrote: January 4th, 2023, 1:58 pm - In menu.c, change "QUICKSAVE" and "QUICKLOAD" to respectively "QUICKSAVE (F6)" and "QUICKLOAD (F9)".

- If the above suggestion is accepted, in seg000.c, change
[...]
(The crux of the idea being that if the menu already includes quicksaving entries and explains its shortcuts, the splash screen is more informative if it mentions the menu.)
Done: https://github.com/NagyD/SDLPoP/commit/ ... bacf6c2dde
Norbert wrote: January 4th, 2023, 1:58 pm - For consistency, in menu.c, edit "Fix quick save in feather mode" to say quicksave (without the space).
Done: https://github.com/NagyD/SDLPoP/commit/ ... 06e74d0025
Norbert wrote: January 4th, 2023, 1:58 pm - Feedback from gcc 11.3.0. In options.c, function load_dos_exe_modifications(), opcode - interestingly, for the "else if" - may be used uninitialized.
Could this be another gcc bug, like this one?
Norbert wrote: January 4th, 2023, 1:58 pm - Feedback from "cppcheck --force --language=c":
[...]
Done: https://github.com/NagyD/SDLPoP/commit/ ... a03e28f66a
Norbert wrote: January 4th, 2023, 1:58 pm [...] SDLPoP's Makefile seems to work fine with -std=c99, so you may consider looking into using that, to make it less likely that coders will ever unknowingly use GNU extensions.
Done: https://github.com/NagyD/SDLPoP/commit/ ... 7939a4145e
David
The Prince of Persia
The Prince of Persia
Posts: 2846
Joined: December 11th, 2008, 9:48 pm
Location: Hungary

Re: SDLPoP; David's open-source port of PoP

Post by David »

David wrote: January 7th, 2023, 7:25 pm
Norbert wrote: January 4th, 2023, 1:58 pm - Feedback from gcc 11.3.0. In options.c, function load_dos_exe_modifications(), opcode - interestingly, for the "else if" - may be used uninitialized.
Could this be another gcc bug, like this one?
And now I too got this warning, when I compiled for release.
It appears with the -O2 option, but not without it.
Strange.

I initialized the variable just to be sure: https://github.com/NagyD/SDLPoP/commit/ ... 01ac95115e
Post Reply