SDLPoP; David's open-source port of PoP

[David]

so I guess these names in types.h are not really correct:

The current version does not have all those names.
Where does "color_3_turquoise" come from, for example?

[David]

From viewtopic.php?p=18158#p18158

I suppose this should also (rarely) happen in the original game? If so, this has to be the most obscure bug that has been found in the game so far! Nice find!

Looks like now it has a challenger... https://github.com/NagyD/SDLPoP/issues/66

[Falcury]

From viewtopic.php?p=18158#p18158

I suppose this should also (rarely) happen in the original game? If so, this has to be the most obscure bug that has been found in the game so far! Nice find!

Looks like now it has a challenger... https://github.com/NagyD/SDLPoP/issues/66

That bug is quite subtle indeed!

so I guess these names in types.h are not really correct:

The current version does not have all those names.
Where does "color_3_turquoise" come from, for example?

It's from this commit (but I only added that to the script branch). I thought that it would be useful to know all of the available color codes, so I simply tried them all and made up names for the missing ones... I did not know at that point that they were the colors of a "standard" 4-bit palette.

[Norbert]

The attached ZIP file is a modification of version 1.16.
I've removed the data/ directory and the DLL files to make the package smaller.

This is only a very minor, incomplete and overall ugly modification.
Don't expect to see anything interesting.

The modification is that the clients send movement input to a server, and a server that sends that movement data to all other clients.
The clients do almost nothing with the movement data, they just display it on the console.

To find the modifications I've made, you can search for "Norbert" in the files.
The changes are also listed below.

* Added:
- client.sh: a simple script that calls the prince executable with "--host=127.0.0.1 --port=2000".
- def.c: some functions that I needed, primarily Send() and Receive().
- pserver.c: waits for and accepts clients, does a basic handshake, and then passes the clients' messages (about movement) to all other clients.

* Changed:
- common.h: added some includes/defines and global variables.
- Makefile: added what I needed to build the pserver executable.
- proto.h: added declarations that I needed for def.c.
- seg000.c: modified in three places, to 1. receive and 2. send movements in do_paused(), and 3. connect to the server in pop_main().

How to test: just start pserver and then run client.sh twice.

Now, here's the problem. In code not included, I started adding additional 'kids' (prince characters) to the game. This results in major changes of the client. Even, for example, a simple change in data.h from "extern char_type Kid;" to "extern char_type Kid[8];" changes loadkid(), which changes loadshad_and_opp(), etc. This is not a problem per se/an sich, but, in my opinion, generally speaking, the layout, comments and formatting of the SDLPoP code is not very pretty. I would like to make all the code more readable. David will know what I mean by this. I've done it with (parts of) CusPop in the past, and I started doing it with his code that exports SNES resources. This would be a lot of work. More importantly, it would change the coding style, and coding styles can be very personal. I could also make mistakes and introduce errors, I've made them in the past - not many, but still. And I'd have to do this with a more recent version of SDLPoP that includes changes that were made since version 1.16. Also, it would essentially mean that other people could not touch the code while I'm changing it, unless they are willing to later merge it by hand. Just some thoughts. Let me know what you think, David, Falcury, others.

[Edit: Oh, alternatively, someone else could change the client to process the server data, of course. This is all very much a work in progress. Even if I'd make the code more readable - at least for me - in the end I may still be unable to make it multiplayer.]

[Falcury]

I very much like the idea!
I couldn't get it to work yet... It seems the code needs some modifications to work correctly on Windows.
I found this and tried to get as far as possible. But I have never done network programming before so I would definitely need to look into this deeper.

Here's how I got it to at least compile on Windows:
- replace sys/socket.h with winsock.h, like so:

Code: Select all

#ifdef _WIN32
  #include <winsock2.h>
  #include <Ws2tcpip.h>
#else
  #include <sys/socket.h>
  #include <arpa/inet.h>
#endif

- replace the calls to fcntl() with something equivalent (I got this snippet from here):

Code: Select all

		/*** Make non-blocking. ***/
#ifdef _WIN32
		u_long iMode=1;
		ioctlsocket(iSSocket,FIONBIO,&iMode);
#else
		//...
#endif

- link with ws_w32 - in my case (as I use CMake), I added it to the "prince" and newly added "pserver" targets in CMakeLists.txt:

Code: Select all

target_link_libraries(prince mingw32 SDL2main SDL2 SDL2.dll SDL2_image SDL2_mixer ws2_32)
target_link_libraries(pserver mingw32 SDL2main SDL2 SDL2.dll SDL2_image SDL2_mixer ws2_32)

If I then run the compiled pserver.exe, I get the error message "[FAILED] Could not create server endpoint: No error!"
If I suppress the error and recompile, it says "[ OK ] Waiting for clients...". Windows also then shows its firewall security dialog.

Running "prince.exe --host=127.0.0.1 --port=2000" then gives me errors again, unfortunately ("Could not create endpoint: No error!"; when suppressed: "Could not connect: No error!"; when this too is suppressed, the program hangs).

I will be very interested to see how this develops!
A general comment/suggestion: perhaps if we want to integrate new code into the codebase, it would be best to stick to David's coding styles / naming conventions, because this limits confusion and the code will probably be the nicest to read if the conventions are used consistently. What do you think?

[Edit: I just reread your comments:

This is not a problem per se/an sich, but, in my opinion, generally speaking, the layout, comments and formatting of the SDLPoP code is not very pretty. I would like to make all the code more readable. David will know what I mean by this. I've done it with (parts of) CusPop in the past, and I started doing it with his code that exports SNES resources. This would be a lot of work. More importantly, it would change the coding style, and coding styles can be very personal. I could also make mistakes and introduce errors, I've made them in the past - not many, but still. And I'd have to do this with a more recent version of SDLPoP that includes changes that were made since version 1.16. Also, it would essentially mean that other people could not touch the code while I'm changing it, unless they are willing to later merge it by hand. Just some thoughts. Let me know what you think, David, Falcury, others.

I personally do like David's style, but as you say it can be very personal. And there are probably many separate elements to it. If you want to discuss the specific points, it will be a bit easier for me to form an opinion then.
Oh, and of course we have git as a merge tool so that should never be the issue, right?] [Edit2: phrasing, sentences are hard]

Some other ideas for what we could do with netcode in SDLPoP, beside multiplayer:
- Clients could download updated Hall of Fame times from a server.
- When players want to enter their name into a hosted Hall of Fame, they could choose to upload their playthrough's savegame, which would include an automatically captured full replay. Then that replay can be played back (faster than real-time) on the server in order to verify the playthrough time.

[Falcury]

I added a new pull request for loading mods from a dedicated "mods/" folder.
I put it in with the scripting feature earlier, but perhaps it is better to deal with the organisation idea separately from that.

[Norbert]

I think it's better to stick with more portable code and to avoid using Winsock; there's no need for it.

I think both poirot and David are exceptionally smart people, but both produce code that's unnecessarily unreadable. Not because the code is difficult, but because they barely seem to put any effort into making it neat. (As in clean, tidy.) It's all just one stream of messy code. Even the most basic structure, like where functions start and end, is absent. Newlines are used very scarcely. The same goes for spaces and brackets. Everything is lower case, variables aren't identifiable by types; everything is shorthand; this list could go on and on.

And there are probably many separate elements to it. If you want to discuss the specific points, it will be a bit easier for me to form an opinion then.

Yeah. How about this: pick one SDLPoP function for me and I'll show you how I would format it. Maybe pick a fairly long function with a lot of different things in it, like variables/assignments/switches/ifs/fors/comments, etc.

[Falcury]

I think it's better to stick with more portable code and to avoid using Winsock; there's no need for it.

Like I said, I'm unfamiliar with network programming... as long as it works on Windows as well as everything else I'm happy!

Let me respond to the style discussion point by point:

I think both poirot and David are exceptionally smart people, but both produce code that's unnecessarily unreadable. Not because the code is difficult, but because they barely seem to put any effort into making it neat. (As in clean, tidy.)

Not to be too defensive, but I think this requires a bit of a rebuttal! Especially as I too try to work in David's style.
The code is not "unreadable", I have no trouble with it. Just like everything it takes getting used to. You're right that the code may not be extensively formatted (with added markings and such) beyond what is necessary - on the other hand, this maximizes the "useful information density" on the screen and, when editing the code, no valuable seconds are wasted except on the functionality...

It's all just one stream of messy code.

On the other hand: the style is dense, which may actually be an advantage at times, if only because more code fits on the screen.

the most basic structure, like where functions start and end, is absent

I am not bothered by this.
(and many IDEs can helpfully display line separators between functions, perhaps use something like that?)

Newlines are used very scarcely. The same goes for spaces and brackets.

In general I think the conventions David uses are good? In what context do you think there should me more newlines/spaces/brackets?

Everything is lower case

I have used CamelCase in the past, now I use lower_case - I found that it really doesn't matter very much. It's just a convention. Muscle memory adapts quickly enough.

variables aren't identifiable by types

In my opinion this is actually a significant advantage. Only useful (identifying) information in variable names means a higher signal-to-noise ratio. It's easy enough to remember what type a variable is.

everything is shorthand

What do you have in mind that is too shorthand?

Again, my apologies if I come across too defensive. You must admit you're being a little evangelical here ;-)
And if course I admit that the code is not perfect! (when is it ever?)

[Norbert]

I don't feel like doing a back and forth about this, sorry.
Just keep the code as is. I don't mind.

[Falcury]

I don't feel like doing a back and forth about this, sorry.
Just keep the code as is. I don't mind.

OK, I hope I wasn't overzealous. I do appreciate your own neat style of course, it certainly has a systematic elegance to it.

I'll try to read up on network sockets.

[Falcury]

I got it working!
Quite awesome to see two instances of the game communicating like this, even if there's not very much to see at present.

I am afraid I did have to use Winsock to make the code portable... apparently there does exist a workaround for Windows (by using Cygwin to compile instead) but I did not try that. Fortunately, it seems the differences between the BSD sockets and Winsock are relatively minor anyway, so they can be abstracted away using the preprocessor.

[Norbert]

Nice.

[Falcury]

A while ago we discussed enabling/disabling the various fixes/enhancements in the SDLPoP.ini file. One of the points that came up was that it was not very clear which option does what exactly.
To try to make this better, I made a version of SDLPoP.ini with added comments to explain each of the options. Hopefully this improves it a bit.
Github PR: link

[David]

For some reason, the dll size is smaller compared to official release from on libsdl.org (746 KiB vs 1023 KiB).

This reminds me of something I saw in some cartoon: They took something apart and put it together again, but some parts were left out.
I hope nothing important was left out in this case...

[Falcury]

I submitted some new bug fixes.
Some of the fixes are for obvious glitches, others are more subtle annoyances that I finally got fed up with!

Here are the descriptions of the bugs:

Fix unintended sword strike immediately after drawing sword
Sometimes, the kid may automatically strike immediately after drawing the sword.
This especially happens when dropping down from a higher floor and then turning towards the opponent.

I think this happens because ctrl1_shift2 may still be -1 in draw_sword(), after which control_shift2 is set to -1 in rest_ctrl1().

Fix retreating out of a room without the room actually changing
By repeatedly pressing 'back' in a swordfight, you can retreat out of a room without the room changing. (Trick 35)

The game waits for a 'legal frame' (e.g. frame_170_stand_with_sword) until leaving is possible;
However, this frame is never reached if you press 'back' in the correct pattern!

Proposed solution: also allow the room to be changed on frame_157_walk_with_sword
Note that this means that the delay for leaving rooms in a swordfight becomes noticably shorter. (feels more responsive)

Fix running jump through tapestry
The kid can jump through a tapestry with a running jump to the left, if there is a floor above it (when you just miss that floor, the kid jumps through the tapestry instead of falling down in front of it)

The proposed solution is to treat tapestries (when approached to the left) as if they are a wall in start_fall().

Fix guards being pushed into walls
Guards can be pushed into walls, because the game does not correctly check for walls located behind a guard.

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)

This happens because in_wall() mistakenly gets called (which in turn happens because Char.curr_col is not immediately updated after jumping into a wall and getting bumped, even though Char.x has been changed) and in_wall() tries to 'eject' the kid from the wall, but in the wrong direction.
The proposed fix is to update Char.curr_col by calling determine_col() in do_fall().