SDLPoP; David's open-source port of PoP

[Falcury]

replay.c:const char replay_version[] = "V1.16b3 ";
seg000.c:const char quick_version[] = "V1.16b4 ";

Those are the version strings of the quicksave file format, and replay file format. They get inserted at the beginning of quicksave and replay files, respectively. Their function is to check whether the formats are compatible, e.g. in load_replay():

Code: Select all

        fread(replay_control, COUNT(replay_control), 1, replay_fp);
        if (strcmp(replay_control, replay_version) != 0) {
            printf("Warning: unexpected replay format!\n");
        }
        fread(quick_control, COUNT(quick_control), 1, replay_fp);
        if (strcmp(quick_control, quick_version) != 0) {
            printf("Warning: unexpected savestate format!\n");
        }

But yeah, that's not the most elegant solution... I wonder what we could do differently here. Right now I'm working on the replay functionality anyway, maybe I'll think of something.
Speaking of replays, I added an "issue" about replays:
https://github.com/NagyD/SDLPoP/issues/91

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

That happened here:
https://github.com/NagyD/SDLPoP/pull/69 ... b8a7b54a1f

My reasoning at the time (from the commit notes:)
The gameplay for many levelsets may benefit from SDLPoP's bug fixes - however, many mods were also designed to be played without the fixes (and many trick mods even rely on the glitches). So whether or not to enable fixes can probably be thought of as a "design decision" by the mod maker? And as each custom levelset can now just use a customized "mod.ini" to easily override this setting, perhaps it isn't really helpful anymore to directly ask the player whether to enable fixes. Because people do tend to expect the original behavior (I think), it probably makes sense to make the default setting "false". Plus, older levelsets without an added mod.ini will then work exactly as expected, by default at least.

[Norbert]

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

That happened here: [...] My reasoning at the time (from the commit notes:) [...]

Okay, I'm glad about this. We discussed several times what exactly we should quality as bugs, and it's nice that the mod customization options now allow for this back-to-the-default solution.

[David]

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.

He did lots of small things, so I'm not sure what to write there.
For example, each line in ChangeLog could tell who did that thing?
Anyway, perhaps I can add the biggest additions, like replay files, the ini file, and the ability to use mod folders.
Basically everything that is "big", i.e. spans over multiple commits.

Maybe in addition to the already used "Author" and "Contributors" add a "Main Contributors" (or "Core") category?
[...]
Other people (like Falcury) made large contributions.

Is there anyone else who could be under "Main Contributors"?
https://github.com/NagyD/SDLPoP/graphs/contributors -- I guess there isn't...
... unless we look at the editor branch, where almost everything (that is not in master) was done Poirot/@ecalot.
But I'd say that should be kept in the Readme on the editor branch. (Until it's eventually merged into master.)

Would it be useful to separate people who contributed code from those who only reported bugs?
But then, some bug reports might be more important than certain code changes.

One I have in mind is "-Wextra".
[...]
Currently it gives mostly -Wunused-parameter warnings.

Well, *some* of these are inevitable, for callback functions.
But most are leftovers from the disassembly, like the drive parameter of open_dat.

Also 3 -Wsign-compare warnings for comparisons between signed and unsigned integers:

Now I added a cast to (int) in the COUNT macro, and that solves this.
I suppose there will be no variables with sizeof > 2 GB (the maximum value of int on 32-bit systems).

Its description: "This enables some extra warning flags that are not enabled by -Wall."

It's funny that "all" does not mean all...

Then there is "-pedantic" which automatically includes "-Wpointer-arith".
[...]
This currently adds lots of warnings.

Those are about zero-size arrays and unnamed unions in types.h.
And it's made worse by the fact that they are listed for every *.c file that includes types.h. (That is, all of them.)
Is there some elegant solution for these?

If I delete the zeroes (except in midi_type) then I get this instead: "invalid use of structure with flexible array member".
But that still means fewer warnings.

Then there is "-Wcast-qual".
[...]
This currently adds just 1 warning, for line 296 of seg000.c, where a cast discards a const qualifier.

Code: Select all

		process_save((void*) quick_version, COUNT(quick_version));
		ok = quick_process(process_save);

And if I change process_save to accept a const void* as a first parameter, then I get a different warning in the next line about incompatible (function) pointer types...

Unfortunately, someone has broken gamepad support.

It could have been this commit.
Could you please compare the behavior before and after that change?


I updated the readme: https://github.com/NagyD/SDLPoP/commit/ ... 6b1f53e82d
I'm currently working on the warnings.

[Norbert]

Basically everything that is "big", i.e. spans over multiple commits.

I think it looks better after your recent edit, more informative.
This also makes it clear(er) that Falcury has made substantial contributions, and which ones.
Should the contributors be sorted alphabetically?

Well, *some* of these are inevitable, for callback functions.

Maybe not. I haven't looked into it, but a quick search for 'callback function Wunused-parameter' gave me a Stack Overflow page that appears to mention several solutions?

Could you please compare the behavior before and after that change?

I think maybe we should stop posting (extensively) in this 30+ page thread, and instead open new threads. I partially blame myself that I've occasionally been contributing in this big thread. With different threads it will become much easier to look up and read back things related to specific subjects. Visitors can also more easily sift through topics, read what they're interested in. For instance, all posts related to gamepad support could have (had) their own thread.

Anyway, you may remember I mentioned the gamepad-related "30000" values in this thread; values that were originally added and used by segra(x). At the time, after some testing with my Xbox 360 controller, I modified the code, including these values. That was December 2015. I remember contacting segra(x) via private message on this forum, to ask him how thoroughly he had tested his own code. However, he hasn't been on this forum since July 2015. The diff that you mention shows me that "30000" values have been re-added.

What would be useful, as I've suggested in the past, is if we had a 3rd person test gamepad support. I don't feel like asking segra(x) on GitHub which gamepad he's using, but regardless, as I see it, he created something that I fixed that he now broke again. This is just by going what the diff shows me. Also, from what I remember my gamepad is the most used game controller in the world. The reason that I'm not testing the behavior before and after is that it won't change that, at least from my perspective, code that I know worked has since been touched (in April 2016) and now I can no longer properly use my game controller.

[Falcury]

This is what I could come up with to make replays work a little better:
https://github.com/NagyD/SDLPoP/pull/92

Notable changes:
- Can open replay files from anywhere (previously, double-clicking replay files would only work from within the SDLPoP root directory)
- Will save replays to a separate replays/ folder with a more informative naming scheme
- No need for replays to have the REPLAY_001.P1R naming pattern anymore; everything in the replays/ folder is listed and cycled through in reverse creation order
- Will automatically load correct data files & configuration for replays that were created with mods in the mods/ folder
- Can cycle between replays that were created with different mods (switch data files on the fly)
- Should still play older replay files

There are some known issues though; most bothersome is that CusPop options used by replays do not always get cleared properly when the replay ends.

[Falcury]

Looking at the network view on GitHub, I see that some people are currently working on PSP/Vita support for SDLPoP:
https://github.com/Cpasjuste/SDLPoP/commits/master
https://github.com/usineur/SDLPoP-Vita/commits/master

Also, on his fork, usineur committed a neat way to drastically decrease the loading time for the OGG music files:
https://github.com/usineur/SDLPoP-Vita/ ... 1387d7f707
https://github.com/NagyD/SDLPoP/pull/93

This speeds up SDLPoP's overall loading time by a lot!

[David]

https://github.com/NagyD/SDLPoP/pull/93
This speeds up SDLPoP's overall loading time by a lot!

Yes, instead of ~1s, "Loading..." now shows only for a blink!

I merged that and the other pull requests.

https://github.com/Cpasjuste/SDLPoP/commits/master

Hm, it has some interesting pictures:
https://github.com/Cpasjuste/SDLPoP/blo ... splash.png -- ConsoleX Bordeaux -- http://www.consolex-bordeaux.fr/
https://github.com/Cpasjuste/SDLPoP/blo ... splash.png -- GekiHEN Homebrew Contest -- http://gekihen.customprotocol.com/

http://gekihen.customprotocol.com/en/pr ... -Of-Persia
Hey, it should mention the authors of the original SDLPoP, don't you think?

[Falcury]

http://gekihen.customprotocol.com/en/pr ... -Of-Persia
Hey, it should mention the authors of the original SDLPoP, don't you think?

So, for this homebrew competition, they submitted a port of the open-source port of the original DOS port

Yeah, it would be nice if they mentioned the original authors. (I guess that is probably not strictly required by the GPL license)
Interestingly, the project page you linked to has the following acknowledgement:

Special thanks :
...
xerpi for his initial SDL2 port
...

It seems the author meant the port of SDL2 to the Vita platform:
https://github.com/xerpi/SDL-Vita

[Norbert]

http://gekihen.customprotocol.com/en/pr ... -Of-Persia
Hey, it should mention the authors of the original SDLPoP, don't you think?

So, for this homebrew competition, they submitted a port of the open-source port of the original DOS port

Yeah, it would be nice if they mentioned the original authors. (I guess that is probably not strictly required by the GPL license)
Interestingly, the project page you linked to has the following acknowledgement:

Special thanks :
...
xerpi for his initial SDL2 port
...

It seems the author meant the port of SDL2 to the Vita platform:
https://github.com/xerpi/SDL-Vita

"Should mention" does not apply to the web page.
The source code itself still includes your name in the various .c files, and the full overview in doc/Readme.txt.
But yeah, it would be nice if the web page would acknowledge the "SDLPoP author/contributors".

[Falcury]

Anyway, you may remember I mentioned the gamepad-related "30000" values in this thread; values that were originally added and used by segra(x). At the time, after some testing with my Xbox 360 controller, I modified the code, including these values. That was December 2015. I remember contacting segra(x) via private message on this forum, to ask him how thoroughly he had tested his own code. However, he hasn't been on this forum since July 2015. The diff that you mention shows me that "30000" values have been re-added.

What would be useful, as I've suggested in the past, is if we had a 3rd person test gamepad support. I don't feel like asking segra(x) on GitHub which gamepad he's using, but regardless, as I see it, he created something that I fixed that he now broke again. This is just by going what the diff shows me. Also, from what I remember my gamepad is the most used game controller in the world. The reason that I'm not testing the behavior before and after is that it won't change that, at least from my perspective, code that I know worked has since been touched (in April 2016) and now I can no longer properly use my game controller.

I got an Xbox 360 game controller today

Naturally I tried it out immediately... I notice that the Y-axis in particular is hard to control. And it is impossible to mix input from the D-pad, axes, etc.
Yeah, your original fix was indeed better!
The Y threshold should perhaps be in the vicinity of 25000 or so? Or we could even calculate the stick's angle and work with that.
Hm, we should perhaps try to map the triggers to "shift" as well?
And the right stick does nothing as of right now, that could control the kid as well, I suppose?

[Norbert]

I got an Xbox 360 game controller today

Nice.

The Y threshold should perhaps be in the vicinity of 25000 or so? Or we could even calculate the stick's angle and work with that.

Personally, I would say: it's either left, right, up or down. No angles. This means you expect the user to move the stick all the way in one direction/to one side. And this means you can use (very) high values.

And the right stick does nothing as of right now, that could control the kid as well, I suppose?

With my code, both joysticks worked like the D-pad. Of course, there's room for improvement. For instance, as the SDL migration guide mentions, using SDL2's Game Controller API would be an improvement.

[Falcury]

OK, I started on the migration to SDL_GameController, I hopefully will be able to finish this tonight and upload a test build.
I already completed the angle code earlier, and I think you will like it! If not, it can be changed again of course.

EDIT:
I attached the test build, with the controller fixes included. (I also merged the other open pull requests into this test build so those changes (#94, #95 can be tried out as well.)
Here is the GitHub PR for the controller changes:
https://github.com/NagyD/SDLPoP/pull/96

I wrote this in the commit notes:

This improves the controller support in several ways, based on suggestions and earlier work by @EndeavourAccuracy and others:

• Migrated to the SDL_GameController API, which gives an abstraction layer over the controller mappings and may thus provide better compatibility with different controllers.
• Buttons and joysticks on the controller no longer interfere with each other's input (each button or axis should have its own state variable)
• Works with the joystick's angle instead of raw X/Y cutoffs
• In general, tweaked the responsiveness of the joystick controls - playing with a controller should be more pleasant. (tested using an Xbox 360 controller)
• Can now also use the triggers (both left and right) as Shift control.
• Start button now restarts the game (Similar to Ctrl+R), Back button restarts the level (Similar to Ctrl+A).

[Norbert]

I did a quick test.

(For GNU/Linux I had to "#include <math.h>" in common.h, and add "-lm" at the end of the second LIBS line in the Makefile. Both are related to atan2().)

To be honest with you, it's nice that you took care of the migration, but, for me, angle code remains a showstopper. The problem is exactly what I fixed when I touched the code myself, namely that the prince misbehaves when I don't exactly move the joystick left and right when turning. Angles don't work in this situation, because they force the player to constantly make sure the controller is being held at an angle that matches the thumb movement. You should be able to replicate the problem yourself by, for instance, quickly turning left-right-left-right-etc. or by doing the same while also running very small distances before turning. Similar problems also arise near ledges (floor edges). Angles are useful in different situations, such as for 360° 3D games, or 2D games where characters can freely move around.

[Falcury]

Hm, in that case, should the sticks be only used for left/right movement perhaps?
One advantage of the angle code is that if you have a large deviation toward the left or right, then the sensitivity to up/down movement is much reduced compared to when the stick is in the center.
But yeah, if you have to move through the center to get to the other horizontal direction, that's a bit more unsafe.
I don't have a lot of trouble with this, maybe because the controller springs back to the center position anyway, or perhaps I just got used to it

(Also, the angles fortunately make it a bit easier to add some 'exception cases' for some movements that I found difficult to execute using the joystick: for instance, running jumps (because the kid may stop running instead of jumping) and crouch hops. Maybe a similar exception case is possible for moving the joystick through the center?)

Edit:
I just tried it out with the right stick and that seems to be harder compared to the left one, the main problem being that the kid will sometimes duck.
(So apparently my right thumb has a tendency to push the stick slightly below the center? Maybe because the right stick is quite low on the controller. If so, yeah, a somewhat higher cutoff value should definitely help to find the responsiveness vs. annoyance sweetspot ).

[Norbert]

[Oh, wrote this while you wrote your post.]

I think maybe the problem is the 'ideal' testing environment you're using (creating for yourself).
Instead of testing what you've implemented with specific separate movements/input, just play the game.
Hang back, grab the controller, and just chill through all the levels, shouldn't take more than a couple dozen minutes.
If you still don't run into issues while doing that, I won't bring up the gamepad controls again.