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).
SDLPoP; David's open-source port of PoP
-
- Vizier
- Posts: 86
- Joined: June 6th, 2004, 7:05 pm
Re: SDLPoP; David's open-source port of PoP
Re: SDLPoP; David's open-source port of PoP
Nah, there was no automatic decompilation.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?
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]);
}
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
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]);
}
-
- Vizier
- Posts: 86
- Joined: June 6th, 2004, 7:05 pm
Re: SDLPoP; David's open-source port of PoP
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.
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.
-
- Vizier
- Posts: 86
- Joined: June 6th, 2004, 7:05 pm
Re: SDLPoP; David's open-source port of PoP
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.
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.
Re: SDLPoP; David's open-source port of PoP
I've merged it.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 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.
Re: SDLPoP; David's open-source port of PoP
Perhaps we could find a solution for this first: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?
If at all possible, by modifying SDLPoP (as opposed to apoplexy). A workaround I suggested:
Re: SDLPoP; David's open-source port of PoP
I changed SDLPoP to use teleport graphics (or any graphics) from data/ if they are not in the DAT files:Norbert wrote: ↑October 29th, 2022, 3:01 pmPerhaps we could find a solution for this first: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?
If at all possible, by modifying SDLPoP (as opposed to apoplexy).
https://github.com/NagyD/SDLPoP/commit/ ... 1a63e2db92
Re: SDLPoP; David's open-source port of PoP
Great.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
I've just tested it (on Linux), in both dungeon and palace, and, indeed, it works.
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").
-
- Vizier
- Posts: 86
- Joined: June 6th, 2004, 7:05 pm
Re: SDLPoP; David's open-source port of PoP
That's a fair change. You can go ahead and delete the //CONTROL_HELD_ALTDIRECTION comment as that's redundant now.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.
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.
Re: SDLPoP; David's open-source port of PoP
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).
Re: SDLPoP; David's open-source port of PoP
I see you already removed it in the pull request below.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 have merged it.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.
You are right, key_states is not written into save states and replays.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?
(See quick_process() in seg000.c.)
This change is always active in the current version.
(I saw your comment only after I merged the PR...)
-
- Vizier
- Posts: 86
- Joined: June 6th, 2004, 7:05 pm
Re: SDLPoP; David's open-source port of PoP
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.
I can write a PR where I make it a toggle and add support for registering quick joystick input.
-
- Vizier
- Posts: 86
- Joined: June 6th, 2004, 7:05 pm
Re: SDLPoP; David's open-source port of PoP
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:
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.
-
- Vizier
- Posts: 86
- Joined: June 6th, 2004, 7:05 pm
Re: SDLPoP; David's open-source port of PoP
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.
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.
Re: SDLPoP; David's open-source port of PoP
I have added similar functionality on the branch "redefine_keyboard2".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.
It adds controls as a separate menu and you can change any key without having to change all the others.
It looks like this: