SDLPoP; David's open-source port of PoP

Open-source port of PoP that runs natively on Windows, Linux, etc.
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: October 9th, 2022, 1:00 pmI have merged it.

I made some further cleanups, I linked to the commits from my comment on the PR.
Those are good changes. Moving variable assignments outside expressions definitely makes the code a lot more readable. I assume that was originally the result of automatic decompilation? A part of me wishes compilers would treat it like syntax error. I've only had it lead to bugs (for instance, by writing = instead of == in an if statement).
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: October 9th, 2022, 3:53 pm Moving variable assignments outside expressions definitely makes the code a lot more readable. I assume that was originally the result of automatic decompilation?
Nah, there was no automatic decompilation.
I wrote all of the initial SDLPoP source with my bare hands, based on the disassembly.
So it was my original (bad) decision to put the ++ and -- right into the expressions.

As an example, here is (the old form of) a loop from draw_game_frame(), seg000.c:

Code: Select all

				while (drects_count--) {
					copy_screen_rect(&drects[drects_count]);
				}
The loop condition comes from this part of the disassembly:

Code: Select all

seg000:0A61 A1 7E 4D                 mov  ax, drects_count
seg000:0A64 FF 0E 7E 4D              dec  drects_count
seg000:0A68 0B C0                    or   ax, ax
seg000:0A6A 75 E5                    jnz  loc_A51
It decrements drects_count before checking its value, which means the decrement was probably within the condition in Broderbund's DOS PoP source as well.
I guess I cared more about being "faithful" to the original code than anything else.

By the way, moveng the decrement out of the condition makes a semantic difference.
The older code decrements drects_count even if it's already 0, so drects_count will be -1 after the loop ends.
That's probably why there is a "drects_count = 0;" shortly after this loop.

In contrast, when the new form of the loop stops, drects_count will be 0.

Code: Select all

				while (drects_count) {
					drects_count--;
					copy_screen_rect(&drects[drects_count]);
				}
I should probably change bare integer conditions like this to have an explicit comparison. (drects_count != 0)
FluffyQuack
Vizier
Vizier
Posts: 80
Joined: June 6th, 2004, 7:05 pm

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

Post by FluffyQuack »

Oh, doing it all by hand sounds pretty hardcore. It is interesting to see what the original source code most likely looked like. A very faithful reverse engineering is pretty neat from a historical standpoint as you get a good glimpse at how it was made originally.

But yeah, simplifying the code does a lot to make it more readable. It reminds me of the Devilution project (Diablo reverse-engineered). They've got two codebases: one which is as faithful as possible to the original source code, and one where they've been refactoring the code like crazy and adding all kinds of new features to the game.
FluffyQuack
Vizier
Vizier
Posts: 80
Joined: June 6th, 2004, 7:05 pm

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

Post by FluffyQuack »

Another PR that simplifies some code related to controller input: https://github.com/NagyD/SDLPoP/pull/292

I would have preferred to change this code to use bitflags or expand the value range the controller variables use (so it doesn't use 1 to signify different states depending on which specific controller variable it is), but I think it would break older replays or savegames if I were to do that so I ended up with the current changes.
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: October 16th, 2022, 7:08 pm Another PR that simplifies some code related to controller input: https://github.com/NagyD/SDLPoP/pull/292
I've merged it.

I further clarified the values of control_x and control_y: https://github.com/NagyD/SDLPoP/commit/ ... 8115e37f22
"CONTROL_HELD" and "CONTROL_HELD_ALTDIRECTION" just felt strange to me.
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 »

atrueprincefanfrom18 wrote: July 6th, 2022, 8:39 am Can the teleports branch be merged into the main/master branch with an added enhancement flag perhaps?
Perhaps we could find a solution for this first:
David wrote: December 25th, 2021, 2:00 pmI think Apoplexy tries to start SDLPoP from the DOS PoP folder, where the teleport graphics don't exist in VDUNGEON.DAT and VPALACE.DAT .
If at all possible, by modifying SDLPoP (as opposed to apoplexy). A workaround I suggested:
Norbert wrote: December 26th, 2021, 12:45 am Probably certain group+variant combos, that DOS doesn't have anyway, should always load from data/ ?
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: October 29th, 2022, 3:01 pm
atrueprincefanfrom18 wrote: July 6th, 2022, 8:39 am Can the teleports branch be merged into the main/master branch with an added enhancement flag perhaps?
Perhaps we could find a solution for this first:
David wrote: December 25th, 2021, 2:00 pmI think Apoplexy tries to start SDLPoP from the DOS PoP folder, where the teleport graphics don't exist in VDUNGEON.DAT and VPALACE.DAT .
If at all possible, by modifying SDLPoP (as opposed to apoplexy).
I changed SDLPoP to use teleport graphics (or any graphics) from data/ if they are not in the DAT files:
https://github.com/NagyD/SDLPoP/commit/ ... 1a63e2db92
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: October 29th, 2022, 5:42 pmI changed SDLPoP to use teleport graphics (or any graphics) from data/ if they are not in the DAT files:
https://github.com/NagyD/SDLPoP/commit/ ... 1a63e2db92
Great. :)
I've just tested it (on Linux), in both dungeon and palace, and, indeed, it works.
atrueprincefanfrom18 wrote: July 6th, 2022, 8:39 am[...] with an added enhancement flag perhaps?
Probably best to have it available by default, similar to the fake tiles and colored torch flames. Having it available by default doesn't impact default PoP1 gameplay, and makes (enhanced) modding easier.

I'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").
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: October 22nd, 2022, 5:59 pmI've merged it.

I further clarified the values of control_x and control_y: https://github.com/NagyD/SDLPoP/commit/ ... 8115e37f22
"CONTROL_HELD" and "CONTROL_HELD_ALTDIRECTION" just felt strange to me.
That's a fair change. You can go ahead and delete the //CONTROL_HELD_ALTDIRECTION comment as that's redundant now.

I made another PR: https://github.com/NagyD/SDLPoP/pull/293

This isn't related to code cleanup, but it fixes an issue with the controls. If you press and release a key within the 83.333ms window between game updates then the key press is ignored. I made the key_states array use bitflags: 0x1 acts the same as the value used to exist (that is, it gets set and removed immediately as the user presses down and releases a key), and 0x2 gets set when the user presses a key but is only removed after the game processes game input.

I think the controls feel more responsive with this change. I feel it's mostly noticeable during combat.

That said, I'm not completely sure if my implementation is the cleanest. At least, it shouldn't interfere with save states and replays. Those store the control_x and related variables rather than key_states, right?

And I just realized I only did this with keyboard controls, and not joystick controls. I can update those too if you think the above implementation is fine.
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 »

FluffyQuack, if that makes gameplay different from DOS, however slightly, it'd probably be best to have the code treat it like the other use_fixes_and_enhancements, if it doesn't already (I haven't checked).
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: October 30th, 2022, 11:59 pm That's a fair change. You can go ahead and delete the //CONTROL_HELD_ALTDIRECTION comment as that's redundant now.
I see you already removed it in the pull request below.
FluffyQuack wrote: October 30th, 2022, 11:59 pm I made another PR: https://github.com/NagyD/SDLPoP/pull/293

This isn't related to code cleanup, but it fixes an issue with the controls. If you press and release a key within the 83.333ms window between game updates then the key press is ignored. I made the key_states array use bitflags: 0x1 acts the same as the value used to exist (that is, it gets set and removed immediately as the user presses down and releases a key), and 0x2 gets set when the user presses a key but is only removed after the game processes game input.

I think the controls feel more responsive with this change. I feel it's mostly noticeable during combat.
I have merged it.
FluffyQuack wrote: October 30th, 2022, 11:59 pm That said, I'm not completely sure if my implementation is the cleanest. At least, it shouldn't interfere with save states and replays. Those store the control_x and related variables rather than key_states, right?
You are right, key_states is not written into save states and replays.
(See quick_process() in seg000.c.)
Norbert wrote: October 31st, 2022, 9:41 am FluffyQuack, if that makes gameplay different from DOS, however slightly, it'd probably be best to have the code treat it like the other use_fixes_and_enhancements, if it doesn't already (I haven't checked).
This change is always active in the current version.
(I saw your comment only after I merged the PR...)
FluffyQuack
Vizier
Vizier
Posts: 80
Joined: June 6th, 2004, 7:05 pm

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

Post by FluffyQuack »

I tested the game in DosBox, and very brief keyboard and joystick input is dropped the same way there, so I think it's safe to say the original Dos release has the same issue. For the heck of it, I tested POP2 as well, and that also has the same issue.

I can write a PR where I make it a toggle and add support for registering quick joystick input.
FluffyQuack
Vizier
Vizier
Posts: 80
Joined: June 6th, 2004, 7:05 pm

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

Post by FluffyQuack »

Finished writing the new PR which fixes dropped gamepad input and makes everything a toggle: https://github.com/NagyD/SDLPoP/pull/294

List of changes:
  • Replaced joy_axis size definition with a define.
  • Added joy_axis_max array that stores the highest reached value for analogue gamepad inputs between gameplay ticks.
  • Replaced variables storing gamepad button inputs with joy_button_states array. The joy_button_states array contain different values depending on if a button is currently held down and if it's been pressed since last gameplay tick.
  • Made input code store status of start and back buttons.
  • Made it possible to progress past SDL-PoP's text splash screen using any button on a gamepad.
  • Added an ini/menu toggle for the dropped input fix.
FluffyQuack
Vizier
Vizier
Posts: 80
Joined: June 6th, 2004, 7:05 pm

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

Post by FluffyQuack »

I experimented with a feature letting you remap controls:


This is very obviously work-in-progress, hence the "blah blah blah" and it only letting you modify keyboard controls right now. Should I try to finish this feature? And also, is this style of UI for it okay? That is, it switching to a black screen where you always have to press the key/button for each unique action in order.
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 9th, 2022, 10:06 pm I experimented with a feature letting you remap controls:
[...]
This is very obviously work-in-progress, hence the "blah blah blah" and it only letting you modify keyboard controls right now. Should I try to finish this feature? And also, is this style of UI for it okay? That is, it switching to a black screen where you always have to press the key/button for each unique action in order.
I 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
redefine_keys.png (2.69 KiB) Viewed 5743 times
Post Reply