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).
Re: SDLPoP; David's open-source port of PoP
Posted: October 9th, 2022, 7:40 pm
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:
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.
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)
Re: SDLPoP; David's open-source port of PoP
Posted: October 10th, 2022, 9:52 pm
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.
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.
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/ ?
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).
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").
Re: SDLPoP; David's open-source port of PoP
Posted: October 30th, 2022, 11:59 pm
by FluffyQuack
David wrote: ↑October 22nd, 2022, 5:59 pmI've merged it.
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
Posted: October 31st, 2022, 9:41 am
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).
Re: SDLPoP; David's open-source port of PoP
Posted: October 31st, 2022, 10:53 am
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.
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...)
Re: SDLPoP; David's open-source port of PoP
Posted: November 1st, 2022, 12:16 am
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.
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.
Re: SDLPoP; David's open-source port of PoP
Posted: November 9th, 2022, 10:06 pm
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.
Re: SDLPoP; David's open-source port of PoP
Posted: November 12th, 2022, 12:41 pm
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.