Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SWORD1: Rewrite main menu code #5332

Merged
merged 4 commits into from Sep 21, 2023
Merged

SWORD1: Rewrite main menu code #5332

merged 4 commits into from Sep 21, 2023

Conversation

AndywinXp
Copy link
Contributor

Ciao! With this PR I'm rewriting the code for the main menu for Broken Sword 1. This is part of the series of changes I'm doing to the engine to have it match 1:1 (or as close as reasonably possible) with the source code/executable, the DOS version in particular.

Changes in the PR

There were several inaccuracies in the current code (which is only inspired by the original code, and has a completely different structure), so I got permission to rewrite it.

This brings us the following changes:

  • Accurate text positioning;
  • Save and Restore "stone bars" now move when scrolling the list;
  • Implement the "Speed" section and setting;
  • Play intro cutscene when selecting Restart from the main menu;
  • During the tombstone screen (the death menu), the correct font is now used;
  • Implement the "Disk full!" message; this is going to be triggered when it won't be possible to save a savestate to the save location (either for disk full or unwritable media);
  • The askForCd() routine is now accurate overall (correct font, palette fading, font spacing).

Did we lose features?

No, we didn't; of course I ran through the whole commit history for control.cpp and kept the following things for which we do not have the source code:

  • Mac version support;
  • PSX support;
  • Interoperability between GMM settings and original menu settings.
  • Save list scrolling with mouse wheel! 馃槃

Tested with?

The changes have been tested with the following versions of the game:

  • UK (PC & PSX);
  • US (PC & Mac);
  • Italian (PC & PSX);
  • Brazilian (PC);
  • Czech (PC);

Finishing thoughts

One more thing, I just want to stress the fact that this has been done on one commit because the changes pertain an entire component, which when separated either break the game (as in, it can't even boot), or more frequently, crash ScummVM. So I judged the changes to be pretty much atomic.

There's still a degree of accuracy improvements which can be done:

  • On the US demo you're not supposed to even open the menu;
  • On the UK demo, the menu works, but performing a save operation should yield a no-op;
  • Font spacing in the demo menu is significantly wider;
  • The PSX version has no "speed" setting, and has a "Delete" savegame feature; the death tombstone screen also offers all the options in this version (except "Save"), instead of just "Restore", "Restart" and "Quit".

I might change the tombstone screen in PSX, but removing the Speed setting seems a bit useless, and the "Delete" feature certainly doesn't seem feasible on our end (we have a routine which removes files, but apparently we have no permission to do so within an engine?).

As for the demo, I can try to get an accurate font spacing, but I'd like to know what do you think about the other two things.

What's next for SWORD1?

The pause button and the audio engine are next!

Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, I have only couple tiny remarks

engines/sword1/control.h Outdated Show resolved Hide resolved
// changed the resource offsets for some files. I have tried EVERYTHING to
// try and discern which file we are dealing with and spectacularly failed.
// The only choice which seems to work correctly is to check the file size,
// as the newer GENERAL.CLU file is bigger. Sorry.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a valid approach, especially taking into account that practically all the official version releases are now known

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, if it works for you then it works for me 馃檪

Header header;
int32 totalSprites;
uint32 spriteOffset[2];
} PACKED_STRUCT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of packed structs, but it is how it was here before...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did that for consistency, but if you prefer I can always move it out of the packed structs section. Let me know 馃檪

@AndywinXp
Copy link
Contributor Author

Thanks for you review, sev; I have commited the changes you asked 馃檪

@AndywinXp AndywinXp force-pushed the sword1menu branch 2 times, most recently from 4544b62 to c97ebcc Compare September 15, 2023 08:30
@sev-
Copy link
Member

sev- commented Sep 20, 2023

Could you please rebase?

@AndywinXp
Copy link
Contributor Author

Sure! I'm out of town right now but I will do so ASAP tonight, I'll let you know when it's done

@criezy
Copy link
Member

criezy commented Sep 20, 2023

I got a crash when testing the changes, but I am unable to reproduce it 馃槙
And thus I have no idea if it is introduced by the changes in this pull request, or it could happen with the current master code as well.

The crash happened when I tried to access the Volumes settings.

==48895==Hint: address points to the zero page.
    #0 0x1009a4a64 in Sword1::MemMan::checkMemoryUsage() memman.cpp:90
    #1 0x1009a41fc in Sword1::MemMan::alloc(Sword1::MemHandle*, unsigned int, unsigned short) memman.cpp:52
    #2 0x1009be5c4 in Sword1::ResMan::resOpen(unsigned int) resman.cpp:257
    #3 0x1009be4a0 in Sword1::ResMan::openFetchRes(unsigned int) resman.cpp:202
    #4 0x1009660ec in Sword1::Control::initialiseVolume() control.cpp:1188
    #5 0x100959774 in Sword1::Control::saveRestoreScreen() control.cpp:442
    #6 0x100957d34 in Sword1::Control::getPlayerOptions() control.cpp:257
    #7 0x100a08f5c in Sword1::SwordEngine::mainLoop() sword1.cpp:834
    #8 0x100a07fc0 in Sword1::SwordEngine::go() sword1.cpp:683
    #9 0x100a0be2c in Sword1::SwordEngine::run() sword1.h:127
    #10 0x10090d1fc in runGame(Plugin const*, Plugin const*, OSystem&, Common::String const&) main.cpp:318
    #11 0x100907c0c in scummvm_main main.cpp:758
    #12 0x1008fbe40 in main macosx-main.cpp:44
    #13 0x1040ad088 in start+0x204 (dyld:arm64e+0x5088)

That line in question is this one:

free(_memListFreeEnd->data);

which likely means that _memListFreeEnd was a null pointer.

@criezy
Copy link
Member

criezy commented Sep 20, 2023

By the way, do you know if this is normal that we don't have a position indicator on the scrollbar, or is this something that was present in the original and that we don't implement in ScummVM?

image

@AndywinXp
Copy link
Contributor Author

I got a crash when testing the changes, but I am unable to reproduce it 馃槙 And thus I have no idea if it is introduced by the changes in this pull request, or it could happen with the current master code as well.

The crash happened when I tried to access the Volumes settings.

Hmm, I fixed a similar bug while developing this, I'll check more closely and let you know...

By the way, do you know if this is normal that we don't have a position indicator on the scrollbar, or is this something that was present in the original and that we don't implement in ScummVM?

Yes, this is completely normal, there is no position cursor graphic either. If there were any I would have made every effort possible to implement it

This now matches the source code and the executable;
this fixes a lot of inaccuracies, implements the tombstone
"death font", implements the Speed setting.

The askForCd() routine is now more accurate as well.
Quoting -sev: "Some chars on some platforms are signed"
鈥/Demo

This bug has apparently been here since forever. We can't use the static
keyword on the _srIdList[] if it gets edited during the Demo or the UK version
and then never brought back the way it was before when either the US or
the localized versions are started up.

This is caused by those families of versions having different resource IDs
for the menu elements (and a different number of resources...).
@AndywinXp
Copy link
Contributor Author

Unfortunately I couldn't reproduce your issue, but I found a 20 years long bug: if I run either the Demo or the UK version of the game, return to launcher, and then run either the US or any other localized version, the latter will crash.

Why?

Because someone used "static" on a resource array which was modified at boot up for the UK and Demo, and never brought back the way it was originally when the other versions were started up 馃檪

Anyway it's fixed now!

@raziel-
Copy link
Contributor

raziel- commented Sep 21, 2023

@AndywinXp

Regarding the scrollbar indicator...

This is just out of curiosity as it will probably be too much work for such a small cause, but would it be possible to extract the "arrow up" and "arrow down" (above and below the scrollbar) icons in your screenshot and use them (glued together) as a scrollbar indicator (maybe adding the need for a sword1.dat file which holds the gfx)?

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Sep 21, 2023

@AndywinXp

Regarding the scrollbar indicator...

This is just out of curiosity as it will probably be too much work for such a small cause, but would it be possible to extract the "arrow up" and "arrow down" (above and below the scrollbar) icons in your screenshot and use them (glued together) as a scrollbar indicator (maybe adding the need for a sword1.dat file which holds the gfx)?

This goes into the realm of pretty elaborated enhancements, and when they are not embarassingly trivial to implement (like scrolling the save screen with the mouse wheel), that's where I stop and leave them for someone else. My mission here is preserving the original behavior as closely as achievable, so I'm afraid this feature would be out of scope.

@raziel-
Copy link
Contributor

raziel- commented Sep 21, 2023

Thank you very much for your time to answer

@sev-
Copy link
Member

sev- commented Sep 21, 2023

Merging this. Thank you very much!

@sev- sev- merged commit e370442 into scummvm:master Sep 21, 2023
7 of 8 checks passed
@AndywinXp AndywinXp deleted the sword1menu branch September 21, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants