SDLPoP; David's open-source port of PoP

[Norbert]

Nice to see that SDLPoP is getting more polished still.
Sometimes I think SDLPoP is flawless, but I guess there's always room for improvement.

[David]

Fix jumping through a wall above a closed gate
By doing a running jump into a wall, you can fall behind a closed gate two floors down. (e.g. skip in Level 7)

I wrote about this some time ago: viewtopic.php?p=15042#p15042
Also the end of this post: viewtopic.php?p=15048#p15048

[Falcury]

Nice to see that SDLPoP is getting more polished still.
Sometimes I think SDLPoP is flawless, but I guess there's always room for improvement.

Yeah Maybe some day we will run out of bugs to fix...

Fix jumping through a wall above a closed gate
By doing a running jump into a wall, you can fall behind a closed gate two floors down. (e.g. skip in Level 7)

I wrote about this some time ago: viewtopic.php?p=15042#p15042
Also the end of this post: viewtopic.php?p=15048#p15048

Yeah, this one was really hard to figure out for me... Very interesting what you describe in your earlier post! I actually didn't look at bumped_floor() at all; in the test room I used, I could also reproduce the glitch if the floor was not present at all. So it seems then that there may be two problems interacting with each other?

The sequence of events is perhaps something like this:
1. Bump occurs, which pushes Char.x back some distance (either the 'appropriate' amount, or some inappropriate amount if a bug occurred in bumped())
2. Char.curr_col is not updated after the bump, because load_fram_det_col() or determine_col() is not called again until later
3. In do_fall(), get_tile_at_char() is called for the purpose of checking that the kid might be inside a wall tile. Because Char.curr_col has not been updated, the game concludes that the kid is inside a wall tile and calls in_wall() to try to eject the kid in a nice and orderly manner.
4. in_wall() has to decide whether to eject the kid forward or backward, and it needs to know how far the kid should be ejected. To determine the direction, distance_to_edge_weight() is called to determine the kid's x-position within the current tile. However, this uses the raw Char.x position and not the (outdated) Char.coll as the 'tile of reference'. In the case of the Level 7 trick, this distance is calculated to be '5'. If the distance to the edge of the tile is 7 or less, in_wall() chooses the forward direction to eject the kid. Note that if the kid is to be ejected in the forward direction, in_wall() requires that there should not be a wall tile in front of the current wall tile (this is why the trick doesn't work if there is a 2-tiles thick wall above the gate!)
5. in_wall() pushes the kid back into the wall.
6. The kid falls out of the ceiling of the wall tile and bumps with the gate tile. The game decides that the kid is behind the gate and should therefore be pushed even further, not back.
7. The kid lands behind the gate.

[Falcury]

Should we pick a release date for the next version of SDLPoP? It's been a while since a new version came out.
End of next month maybe?

[Norbert]

Should we pick a release date for the next version of SDLPoP? It's been a while since a new version came out.
End of next month maybe?

Good idea.
David could process the last open merge requests, and then the ChangeLog.txt needs lots and lots of extra lines.

[Falcury]

Good idea.
David could process the last open merge requests, and then the ChangeLog.txt needs lots and lots of extra lines.

I wonder what we should do with the in-game editor and scripting feature pull requests.

I think my scripting feature may (still) be too low-quality to be merged... I'll be grateful for any sharp criticisms or ideas (I will no doubt have a 'blind spot' for my own code) Not to mention, the feature introduces an extra dependency, which is an added cost.

David asked about merging the editor branch on GitHub, I wrote a post over there:
https://github.com/NagyD/SDLPoP/issues/ ... -245592687

[Norbert]

Oh, and David, maybe add an icon.

One way would be to add in seg009.c, after SDL_CreateRenderer:

Code: Select all

SDL_Surface *imgicon;
imgicon = IMG_Load ("SDLPoP_icon.png");
SDL_SetWindowIcon (window_, imgicon);

This requires a SDLPoP_icon.png image in the same directory as the executable.
Of course, a simple code change would allow the icon to reside in data/ or whatever.
And maybe "SDL_Surface *imgicon;" should be moved into data.h.

[Norbert]

I may have asked this in the past, but I don't remember (getting) an answer.
Why are the DAT files DIGISND* and GUARD* in SDLPoP's root directory?
Also, data/ seems to have GUARD* directories, but not DIGISND* directories.

Also, was this (mirror) blue color a known bug with v1.16, or...?

[Falcury]

I may have asked this in the past, but I don't remember (getting) an answer.
Why are the DAT files DIGISND* and GUARD* in SDLPoP's root directory?
Also, data/ seems to have GUARD* directories, but not DIGISND* directories.

Currently, SDLPoP can only open DAT files if they are in the main folder. If the DAT file is not found there, it will try to find the data files in the corresponding folder in the data/ directory, however this only works if the *.DAT is actually a folder and not a regular file.

There is a bug with the GUARD* directories under Mac OS X (the PNG guard images get loaded as-is in greyscale, the color palette is not applied). However this bug does not occur if the .DAT files are loaded instead. That's why these were added in the main directory, to override the GUARD* folders as a workaround.
And you're right, the DIGISND*.DAT files do not have counterparts in the data/ directory.

It may be cleaner to allow to have (overriding) DAT files in the data/ folder as well.
Of course it is impossible to have directories and regular files that share the same name, which is a bit of an issue. We could perhaps work around this by dropping the .DAT extension for the folders.

EDIT:
I implemented the changes proposed above:
https://github.com/NagyD/SDLPoP/pull/90

Also, was this (mirror) blue color a known bug with v1.16, or...?

Yes, this is due to an SDL bug that swapped the blue and red color components in SDL_FillRect. This was fixed in SDL 2.0.4, which caused the bug to reappear because we had also swapped the color components as a workaround.
David made the workaround conditional on the SDL2 version, so it's all fixed now:
https://github.com/NagyD/SDLPoP/blob/3d ... 09.c#L2246

[Norbert]

the PNG guard images get loaded as-is in greyscale, the color palette is not applied

I see this was discussed here and further. Apparently people have ran into this at least since 2011. I haven't looked into all this, but if this is a bug in the OS X version of SDL2, does it have a bug report and/or did SDL 2.0.4 (of Jan. 2016) fix this?

I implemented the changes proposed above: [...]

Still fewer files in the root directory, nice improvement.

David made the workaround conditional on the SDL2 version, so it's all fixed now: [...]

Okay.

Some random remarks/questions/suggestions about David's main branch:
- SDL's DLL files are no longer part of the default package?
- Is src/unistd_win.h necessary, it has very little content.
- In the original game, where do the fonts (data/font/) reside, the executable, and, if so, does this mean content was scrapped from (or commented out in) one of the .c files?
- Maybe remove "levels-test" and then rename "levels-original" to "LEVELS"?
- The filename NMakefile seems weird to me. Makefile appears to be for Linux, maybe NMakefile should be renamed to Makefile.win?
- On a related note, Makefile says "Makefile created by Dev-C++ 4.9.9.2" but is that really true, was Dev-C++ used to generate a Linux Makefile?

[David]

- SDL's DLL files are no longer part of the default package?

I'm not sure what you mean.
The DLLs are present in the official binary releases (ZIP files), but they were never in the Git repository.

- Is src/unistd_win.h necessary, it has very little content.
- The filename NMakefile seems weird to me. Makefile appears to be for Linux, maybe NMakefile should be renamed to Makefile.win?

They were added by @diddledan for "Visual C++ (NMake) support": https://github.com/NagyD/SDLPoP/commit/ ... 14796fb55d

By the way, Makefile.win is the name of the Makefile created by Dev-C++.

- On a related note, Makefile says "Makefile created by Dev-C++ 4.9.9.2" but is that really true, was Dev-C++ used to generate a Linux Makefile?

Um, no. It generated a Windows Makefile, and I rewrote that to work on Linux.
Now I removed it: https://github.com/NagyD/SDLPoP/commit/ ... 9bbe13bdb9

- In the original game, where do the fonts (data/font/) reside, the executable, and, if so, does this mean content was scrapped from (or commented out in) one of the .c files?

In the original game, the font is in PRINCE.EXE.
In SDLPoP, the font data is present in both data/font/ and seg009.c, as byte hc_font_data[].
The latter was added as a fallback in case SDLPoP is started from a folder where there is no data/. (A mod's folder for example.)
It was added here: viewtopic.php?p=15704#p15704

- Maybe remove "levels-test" and then rename "levels-original" to "LEVELS"?

The levels-test has some test-case levels, I'm not sure how relevant are they nowadays.
Also, the only way to start them is to copy them over a file in data/LEVELS.DAT/.

And you're right, the DIGISND*.DAT files do not have counterparts in the data/ directory.

They are there because currently, SDLPoP can load sound effects only from DAT files.
I guess this should be changed...

[Norbert]

For Linux, maybe add some additional CFLAGS.

One I have in mind is "-Wextra".
Its description: "This enables some extra warning flags that are not enabled by -Wall."
Currently it gives mostly -Wunused-parameter warnings. Also 3 -Wsign-compare warnings for comparisons between signed and unsigned integers:
- seg008.c line 715
- seg009.c, line 120
- seg009.c, line 1675

Another I have in mind is "-Wshadow".
Its description: "Warn whenever a local variable or type declaration shadows another variable, parameter, type, class member (in C++), or instance variable (in Objective-C) or whenever a built-in function is shadowed."
Currently, it adds several warnings. Most are for dathandle, tile_row and tile_col. The others are:
- level in seg000.c and seg003.c,
- trob_type in seg007.c, and
- rect_top in seg009.c.

Then there is "-pedantic" which automatically includes "-Wpointer-arith".
Its description: "Issue all the warnings demanded by strict ISO C and ISO C++ [...] Where the standard specified with -std represents a GNU extended dialect of C, such as gnu90 or gnu99, there is a corresponding base standard, the version of ISO C on which the GNU extended dialect is based. Warnings from -Wpedantic are given where they are required by the base standard."
This currently adds lots of warnings.

Then there is "-Wcast-qual".
Its description: "Warn whenever a pointer is cast so as to remove a type qualifier from the target type. For example, warn if a "const char *" is cast to an ordinary "char *". Also warn when making a cast that introduces a type qualifier in an unsafe way."
This currently adds just 1 warning, for line 296 of seg000.c, where a cast discards a const qualifier.

Currently, "-Wunused-result" is already included (probably via "-Wall").
You may want to disable this with "-Wno-unused-result", I also do/did this for apoplexy.
Alternatively, you could modify code to prevent the warnings.
Currently it gives 9 warnings for all fread() usage in replay.c, and for the 2 last fscanf() lines in options.c.

[Norbert]

When I grep "1.16" in the source files, I get:
config.h:#define WINDOW_TITLE "Prince of Persia (SDLPoP) v1.16"
replay.c:const char replay_version[] = "V1.16b3 ";
seg000.c:const char quick_version[] = "V1.16b4 ";

The game version should probably just be defined once.

Then use either...

#define VERSION "1.16"
#define WINDOW_TITLE "Prince of Persia (SDLPoP) v" VERSION

...or:

#define VERSION "0.16"
#define WINDOW_TITLE "Prince of Persia (SDLPoP) v"
char sTitle[100];
snprintf (sTitle, 100, "%s%s", WINDOW_TITLE, VERSION);
window_ = SDL_CreateWindow(sTitle, <etc>

If there really need to be separate replay and quick versions, in my opinion these shouldn't be very-similar-to-but-not-the-same-as the game version.

[Norbert]

Maybe in Readme.txt, Falcury's "improvements, additions" could be more specific.
Readers may be curious what exactly the improvements and additions in question are.
Not all readers have been following discussions on this forum, not all have seen the pull requests.

Maybe in addition to the already used "Author" and "Contributors" add a "Main Contributors" (or "Core") category?
Clearly some people (like me) are not main contributors, I've done almost nothing.
Other people (like Falcury) made large contributions.
Then again, it may be difficult to differentiate between these two contributor groups.
Maybe it's mostly that Falcury's "improvements, additions" description seems too generic to me.

Also, for two people listed in the overview, mfn and zaps166, the file does not give any information about their contributions.
What did these people do? I suggest adding this information.

[Norbert]

Just something I noticed: the main branch does not ask if improvements should be enabled.

Maybe Ctrl-V should also show the SDLPoP version? (Although it's already on the title bar.)

In doc/Readme.txt, the "LICENSE" section, maybe either end the sentence with "[...] and src/GPLv3.h." or after "terms" add ", version 3 or later". The reason I'm suggesting this is that gpl-3.0.txt itself does not specify whether the code is GPL3 or GPL3+ licensed.

In mods/mods.txt, step 2 says "Make sure that the name of the mod's folder is correct." It's not clear to me as a reader what correct entails in this context. What would the writer qualify as incorrect? Maybe step 2 can be removed and in step 3 "the name of the mod" can be changed to "the name of the folder with the mod's files" of something similar.

In doc/Readme.txt, maybe add to "a number from 1 to 14 -- Start the given level" that this only works in conjunction with the megahit cheat.

Unfortunately, someone has broken gamepad support. I just retested both the current main branch and the 1.16 release, and the later works while the former has various serious bugs. It's basically in an unusable state. Example: now and then I can literally press left on the D-pad when the prince is facing right and then he just climbs up a ledge on the right - totally different from what I instructed the game to do.

Related, in doc/Readme.txt, the "Gamepad equivalents" section, it could mention the joystick (which, by the way, also worked with 1.16 but no longer does) and where it says...
"left/right/up/down = left/right/up/down"
...it should probably say something like...
"left/right/up/down = D-Pad"
...or:
"arrows = D-Pad"
(In fact, "arrows = D-Pad/joysticks", because the joysticks used to work like the D-pad.)