SDLPoP v1.22 release candidate

[David]

Here is the release candidate of SDLPoP v1.22, attached to the post.
The ZIP contains the Windows binaries and the source as usual.

The list of changes since the last release can be found here: https://github.com/NagyD/SDLPoP/blob/8d ... #L564-L643

I didn't yet mark this release with a tag in Git. I will do so when the release will be final.

[Norbert]

I'll add two posts with feedback.
This first includes gcc warnings that may or may not be of use to you.

--------------------
seg002.c

FIRST comparison is always true due to limited range of data type

(line 132) if (right_guard_tile >= 0 && right_guard_tile < 30) {
(line 142) if (left_guard_tile >= 0 && left_guard_tile < 30) {
(line 150) else if (left_guard_tile >= 0 && left_guard_tile < 30) {
(line 336) if ((kid_room_m1 >= 0 && kid_room_m1 <= 23) &&
--------------------
seg006.c

comparison of unsigned expression >= 0 is always true

(line 753) if (COUNT(bogus) + xpos >= 0) {
--------------------
seg008.c

this statement may fall through

(line 1578) if (united_with_shadow && (united_with_shadow % 2) == 0) goto shadow;
(line 1579) case 2: // Guard
--------------------
common.h

comparison of unsigned expression < 0 is always false

(line 59) #define MAX(a,b) ((a)>(b)?(a):(b))
- in expansion of macro ‘MAX’ seg009.c line 2568
- in expansion of macro ‘MAX’ seg009.c line 2603
--------------------
seg009.c

this statement may fall through

(line 3509) window_resized();
(line 3512) case SDL_WINDOWEVENT_EXPOSED:
--------------------
midi.c

this statement may fall through

(line 471) if (event->sysex.length == 7) {
(line 477) case 0xFF: // Meta event
--------------------

[Norbert]

Second post, with some random things that may or may not be useful to you.

--------------------
seg000.c
says
const char quick_version[] = "V1.16b4 ";
I'm not sure if that's still accurate/useful/in use.
--------------------
PR has only guards/
SDLPoP/ has GUARD/ GUARD1/ GUARD2/ GUARD.DAT GUARD1.DAT GUARD2.DAT
I'm not sure if all these are necessary.
--------------------
data/levels-original/
data/levels-test/
I'm not sure if that should be in data/ or doc/, and whether it's in use or not.
--------------------
doc/mod.ini
I think it's outdated. Maybe remove and modify whatever was pointing to that file to say use SDLPoP.ini as mod.ini and then etc.
--------------------
Readme.txt
"You need to get the music from here: (38 MB)"
That's actually 42 MB these days, as ogg_MT-32/ was added to the ZIP package.
--------------------
When I manually increase the gcc warnings, I get lots of warnings about shadow declarations. That local variables shadow global variables. One reason this is the case is because lots of one-character variables are (re)used, such as i, n, j, m, etc.
--------------------
SDLPoP.ini says
; The base directory where SDLPoP will look for custom levelsets. (default = mods)
mods_folder = mods
And indeed it has: mods/mods.txt
SDLPoP.ini also says
; The folder where replays will be kept.
replays_folder = replays
But it has no: replays/replays.txt (or even replays/)
The above file could be added, so users know where to move downloaded .p1r files.
It could simply point to Readme.txt, sections "Viewing or recording replays" and "REPLAYS".
--------------------
Related to the above.
replays-testcases/
should probably be
replays/testcases/
or
doc/replays-testcases/
At least in my opinion.
Of course, if modified, documentation should also be changed to refer to the correct (new) directory.
--------------------

Regardless, it's nice to see that a new SDLPoP release is nearby.
Great project.

[David]

This first includes gcc warnings that may or may not be of use to you.

Let's see what should I do with each.

By the way, if I use the -Wextra switch (in Dev-C++) then I also get a bunch more errors, about unused parameters, and about comparisons between signed and unsigned integers.
But it doesn't warn about fallthroughs. If I try to use -Wfallthrough then it says this option is "valid for Java but not for C"!?

seg002.c

FIRST comparison is always true due to limited range of data type

(line 132) if (right_guard_tile >= 0 && right_guard_tile < 30) {
(line 142) if (left_guard_tile >= 0 && left_guard_tile < 30) {
(line 150) else if (left_guard_tile >= 0 && left_guard_tile < 30) {
(line 336) if ((kid_room_m1 >= 0 && kid_room_m1 <= 23) &&

I will change those variables to a signed type.

seg006.c

comparison of unsigned expression >= 0 is always true

(line 753) if (COUNT(bogus) + xpos >= 0) {

I will cast the result of COUNT to int.
Maybe the cast should be in the definition of COUNT.

seg008.c

this statement may fall through

(line 1578) if (united_with_shadow && (united_with_shadow % 2) == 0) goto shadow;
(line 1579) case 2: // Guard

This fallthrough is intentional, without it the prince is not drawn.
I will add a comment.

common.h

comparison of unsigned expression < 0 is always false

(line 59) #define MAX(a,b) ((a)>(b)?(a):(b))
- in expansion of macro ‘MAX’ seg009.c line 2568
- in expansion of macro ‘MAX’ seg009.c line 2603

The result of strnlen should be cast to int.

seg009.c

this statement may fall through

(line 3509) window_resized();
(line 3512) case SDL_WINDOWEVENT_EXPOSED:

It seems to work with or without a break, as resizing the window will send an exposed event as well.
Additionally, nothing seems to change even if I remove the update_screen() call there.

midi.c

this statement may fall through

(line 471) if (event->sysex.length == 7) {
(line 477) case 0xFF: // Meta event

I think this is unintentional.
The two branches even access two different members of the same union, event->sysex and event->meta.
I will add a break.


Here are my changes: https://github.com/NagyD/SDLPoP/commit/ ... 26c26f325a

[Norbert]

doc/Readme.txt
http://hobbes.nmsu.edu/h-search.php?key=sdlpop
appears to have changed to
https://hobbes.nmsu.edu/?search=sdlpop
or, with index.php,
https://hobbes.nmsu.edu/index.php?search=sdlpop

[David]

doc/Readme.txt
http://hobbes.nmsu.edu/h-search.php?key=sdlpop
appears to have changed to
https://hobbes.nmsu.edu/?search=sdlpop
or, with index.php,
https://hobbes.nmsu.edu/index.php?search=sdlpop

Indeed, thank you for checking the links.

This link is for the OS/2 port. I was wondering if the Readme should link to SDLPoP ports which are not supported officially (i.e. for systems I don't have).

If I shouldn't link to them then I would remove the OS/2 port.
Although I don't have Mac OS X, Falcury does, so it might count as (semi-)officially supported.

If I should link to them then there are a few more unofficial ports which I could add:
* AmigaOS4: http://www.os4depot.net/?function=showf ... sdlpop.lha
* MorphOS: https://www.morphos-storage.net/?id=1570367
* RetroPie: https://retropie.org.uk/docs/SDLPoP/
* GameShell: viewtopic.php?f=126&t=4252
* Snap: https://snapcraft.io/sdlpop
* Brew: https://formulae.brew.sh/formula-linux/sdlpop
etc.
And they should be under an "unofficial ports" subheader.

[Norbert]

By the way, maybe you missed this post?
Although some of that may have been mentioned or discussed in the past.
I can imagine that maybe you don't feel like going into it again.

[David]

By the way, maybe you missed this post?
Although some of that may have been mentioned or discussed in the past.
I can imagine that maybe you don't feel like going into it again.

I have seen it, but I felt that writing one long reply to a long post (about the gcc warnings) would be enough for that day.

seg000.c
says
const char quick_version[] = "V1.16b4 ";
I'm not sure if that's still accurate/useful/in use.

This is written into the header of QUICKSAVE.SAV on quicksave.
On quicksave, SDLPoP compares the header of QUICKSAVE.SAV to this string, and fails if they are different.

PR has only guards/
SDLPoP/ has GUARD/ GUARD1/ GUARD2/ GUARD.DAT GUARD1.DAT GUARD2.DAT
I'm not sure if all these are necessary.

Originally SDLPoP had only the folders.
I added the DATs because on some systems, loading the paletted images from the GUARD folder results in RGB images.
See here: viewtopic.php?p=15676#p15676 and viewtopic.php?p=19603#p19603

SDLPoP needs paletted images, because the palette of the guard images must be changed every time the player meets a guard of a different color.
That can't be done with RGB images.
This problem does not occur with images loaded from DATs, because they are created by SDLPoP explicitly as paletted images.

data/levels-original/
data/levels-test/
I'm not sure if that should be in data/ or doc/, and whether it's in use or not.

I used the files in levels-test/ in the past, by copying them over files in data/LEVELS/.

doc/mod.ini
I think it's outdated. Maybe remove and modify whatever was pointing to that file to say use SDLPoP.ini as mod.ini and then etc.

mod.ini has all the options commented out, and it also has a different intro text.
Perhaps neither is necessary?

Readme.txt
"You need to get the music from here: (38 MB)"
That's actually 42 MB these days, as ogg_MT-32/ was added to the ZIP package.

That part is struck out (<s>...</s>), because SDLPoP can play music from MIDISND*.DAT files and the ZIP is not needed.
However, people might want to download the ZIP to play with MT-32 music, so we could add that.

I was thinking of something like this:

Spoiler: show

Code: Select all

Where is the music?
----------------------
<s>
Since version 1.13, the game supports loading music from the data/music folder.
Until 1.15, music was not included in releases because it is very big, and it does not change between SDLPoP versions.

Since version 1.15, music is included.
</s>

Since version 1.18, SDLPoP can play music from the MIDISND*.DAT files and OGG files are not included.

You can also play with MT-32 music.
<br>You can get it here: https://www.popot.org/get_the_games.php?game=1
<br>Copy the OGG files from ogg_MT-32 to the data/music folder.

When I manually increase the gcc warnings, I get lots of warnings about shadow declarations. That local variables shadow global variables. One reason this is the case is because lots of one-character variables are (re)used, such as i, n, j, m, etc.

Should I just append an "l_" prefix to each such local variable?

About the one-letter variables:
They appear in errors like this:

Code: Select all

stb_vorbis.c [Warning] declaration of 'z' shadows a previous local [-Wshadow]
stb_vorbis.c [Warning] declaration of 'c' shadows a previous local [-Wshadow]
stb_vorbis.c [Warning] declaration of 'n' shadows a parameter [-Wshadow]

Which means:
1. In this case, locals shadow other locals, not globals.
2. These are in stb_vorbis.c, which is not my code, so maybe I shouldn't change it.

SDLPoP.ini says
; The base directory where SDLPoP will look for custom levelsets. (default = mods)
mods_folder = mods
And indeed it has: mods/mods.txt
SDLPoP.ini also says
; The folder where replays will be kept.
replays_folder = replays
But it has no: replays/replays.txt (or even replays/)
The above file could be added, so users know where to move downloaded .p1r files.
It could simply point to Readme.txt, sections "Viewing or recording replays" and "REPLAYS".

Done: https://github.com/NagyD/SDLPoP/commit/ ... 051d49565e
Should I create a screenshots/screenshots.txt as well?

Related to the above.
replays-testcases/
should probably be
replays/testcases/
or
doc/replays-testcases/
At least in my opinion.
Of course, if modified, documentation should also be changed to refer to the correct (new) directory.

I don't know.

Regardless, it's nice to see that a new SDLPoP release is nearby.
Great project.

Thank you!

[Norbert]

Done: https://github.com/NagyD/SDLPoP/commit/ ... 051d49565e

Thanks. I think that's quite useful and user-friendly.

Should I create a screenshots/screenshots.txt as well?

Personally, I'd say no, because there's no reason for users to ever manually move content into the screenshots/ directory.

Related to the above.
replays-testcases/
should probably be
replays/testcases/
or
doc/replays-testcases/
At least in my opinion.
Of course, if modified, documentation should also be changed to refer to the correct (new) directory.

I don't know.

It feels like an illogically prominent place, the top-level directory, for content that is for development purposes only and will rarely ever be used. I believe to be more logical either of the alternatives that I suggested. It's been quiet on the forum as of late, but maybe another forum user could chime in?

If I should link to them then there are a few more unofficial ports which I could add: [...]

Funnily enough, depending on the context, SDLPoP itself is both official and unofficial (as a port). I think maybe a solution is to create a forum topic on the SDLPoP board that lists the ports, and then simply point to that topic from the README. This way those who're interested can find the ports, while at the same time it's not an 'official' listing that's distributed, allowing you to easily and quickly update the overview if necessary.

[David]

replays-testcases/
should probably be
replays/testcases/
or
doc/replays-testcases/

Done: https://github.com/NagyD/SDLPoP/commit/ ... 7fb9ac8623

data/levels-original/
data/levels-test/
I'm not sure if that should be in data/ or doc/, and whether it's in use or not.

I have deleted them: https://github.com/NagyD/SDLPoP/commit/ ... d14cb0db8a

[David]

I think maybe a solution is to create a forum topic on the SDLPoP board that lists the ports, and then simply point to that topic from the README.

Done.
Topic: viewtopic.php?f=126&t=4728
Commit: https://github.com/NagyD/SDLPoP/commit/ ... bfe5761907

[Norbert]

Should be ready now, I think.
Maybe a new release this coming weekend?

[atrueprincefanfrom18]

Should be ready now, I think.
Maybe a new release this coming weekend?

Maybe after this bug is solved?

Mod: Mini Underground
Level 1. The guard doesn't seem to stop after the Prince touches the tapestry as shown in this video.

Here's the p1r file. Tried in the latest one available on GitHub (The bug still exists in the 1.20 release).

bug_mini_underground.p1r

(4.47 KiB) Downloaded 72 times

Can this be fixed?

[David]

Mod: Mini Underground
Level 1. The guard doesn't seem to stop after the Prince touches the tapestry as shown in this video.

You need to compile with FIX_DOORTOP_DISABLING_GUARD disabled in config.h.
And I should make this option configurable in the INI and the pause menu, so it can be disabled by default, but can be enabled by the user.

[David]

And I should make this option configurable in the INI and the pause menu, so it can be disabled by default, but can be enabled by the user.

Done: https://github.com/NagyD/SDLPoP/commit/ ... 8944e5d568