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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AGS: AGS engine for ScummVM #2738

Merged
merged 215 commits into from Feb 7, 2021
Merged

AGS: AGS engine for ScummVM #2738

merged 215 commits into from Feb 7, 2021

Conversation

@dreammaster
Copy link
Member

@dreammaster dreammaster commented Jan 18, 2021

It's been a long time coming, but I present my reimplementation of the AGS standalone interpreter for ScummVM. At this point in time, I've played through the entirety of my old Black Cauldron remake without any graphic glitches, though I haven't exactly plumbed the depths of the game nor the AGS GUI interface beyond saving and loading.

I'd like to note the following points in no particular order:

  • I already asked permission to do this in the AGS forums late last year, so this implementation can be considered to be done with the blessings of those that cared enough to respond to me at the time. AGS is currently in a good spot for doing a ScummVM implementation.. it's in transition between 3.5.x and 4.0, which will introduce a bunch of structural changes, apparently. So there wasn't rapid commits being done to the codebase that I'd need to try and keep up with.

  • Unfortunately, it does mean that Fuzzie's previous attempt will go to waste. I originally did a few experiments with it, but encountered some crashes I didn't know how to fix. Plus, basing my implementation based on the current standalone interpreter meant that it'd be able to play the up-to-date 3.5 games that Fuzzie's wouldn't, even if were further polished and bugfixed.

  • Namespaces: Though it's a bit unusual, the engine introduces two namespaces: AGS as well as AGS3. The existing standalone codebase had a lot of stuff in the global namespace, and within that it had an AGS namespace of it's own. So I didn't want to just wrap everything in an AGS namespace and end up with a AGS::AGS namespace. Since there's always the strong possibility of AGS4 support in ScummVM as well going forward, I decide to wrap all the existing code in an AGS3 global namespace, and keep the core engine class and support classes in a general AGS namespace.

  • As I mentioned, though my test game is completable, there's still a bunch of work to do. Savegames work, though I haven't yet spent time hooking up volume control to ScummVM. Similarly, other things like selecting a savegame from the ScummVM launcher.

  • Plugins are going to need to be re-implemented in C++ code, obviously, since some of the plugins were only ever released for Windows. I've already obtained source code for AGSCreditz from the original author. Currently, the AgsCreditz class has only stubs for all of the plugin's methods, so expect a lot of unused locals warnings from them. These will go away as I re-implement the original Delphi as C++.

  • Global variables. There's currently a lot of global variables that'll need to be refactored so they'll be properly initialized, and the engine isn't instantiating a bunch of classes on startup in static builds. I'll likely create a dummy "Globals" class to contain them all and use a short macro to simplify accessing them.

  • Project structure: Apart from a bunch of core classes, the project has several subfolders. Shared represents the "Common.Lib" in the original codebase. It used a Common namespace, and is shared by the Editor and runtime. However, since I was getting way too many conflicts between it and the ScummVM Common namespace, early on I renamed it all to Shared. The engine/ folder contains the "Engine.App" project of the original. plugins/ contains the new hierarchy I've started for reimplementing the different AGS plugins in C++ code. Currently, it only has stubs for the AgsCreditz plugin. And finally, lib/ contains subfolders for various libraries AGS needs. Rather than importing everything it needed, I took the route of just implementing stubs for what was needed, and then fleshing it out as required. This means there's less code bloat, and particularly for graphics it already seamlessly uses the ScummVM surface classes, which made rendering stuff to the screen a whole lot easier.

I think that covers everything, but if there are any questions let me know.

More stuff:

  • The engine also adds a new WeakPtr class in common/ptr.h, which necessitated some minor changes for code sharing to SharedPtr
@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Jan 18, 2021

DeepCode's analysis on #c0abf0 found:

  • ⚠️ 14 warnings, ℹ️ 7 minor issues. 👇

Top issues

Description Example fixes
Using strcpy can lead to severe buffer overflow vulnerabilities. Use the safe alternative strncpy instead. Occurrences: 🔧 Example fixes
Leaking memory. ScriptDrawingSurface is allocated on the heap and never freed. In function InitAndRegisterGameEntities. Occurrences: 🔧 Example fixes
The result of malloc, which may return null flows to the first argument of strcpy. This could result in undefined behavior. Consider adding a check for nullness. Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@raziel-
Copy link
Contributor

@raziel- raziel- commented Jan 18, 2021

Wheee, awesome, can't wait to test my AGS library of games

@dreammaster dreammaster force-pushed the dreammaster:ags branch from 3d7c692 to 9815789 Jan 19, 2021
@criezy
Copy link
Member

@criezy criezy commented Jan 19, 2021

I tried a few games, and none of them is working. Here are the various errors I am getting:

  • The Black Cauldron remake: it freezes at starts trying to allocate a huge amount of memory in ReadDialogs (main_game_file.cpp on line 323). The content of the buffer after the call to decrypt_text() seems to be corrupted.

image

which results in

image

  • Gemini Rue, Kathy Rain, Quest for Infamy: Error: TODO: Handle 32-bit pointers in ScummVM if needed!
  • The Blackwell Legacy: !Error running function 'game_start': Error: Null pointer referenced
  • King's Quest II AGDI: Error: TODO: write_screen_shot_for_vista!
  • A Golden Wake: Assertion failed: (_fonts[_size]), function getFont, file engines/ags/lib/alfont/alfont.cpp, line 33.

I will stop there. I understand this is still early and there is more work to do. You mention though that the Black Cauldron remake is working for you. So I am wondering why it fails for me.

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jan 20, 2021

Thanks for the report. It's definitely weird that Black Cauldron doesn't work for you. I just double-checked and it still works for me, so I haven't broken anything with my latest commits. Remind me what system you're running. Particularly if it's big endian, it could be indicative of some endian issues in the codebase. Alternatively, I do know that I actually originally did two releases of Black Cauldron.. a second one a few years later due to some reported issues loading the source code into the latest (at that time) editor. Though if it's a difference between versions I'd presume the md5 wouldn't have matched.

Just in case, I did a full md5 of the entire bc.exe, and it gives me 89db7324d34817972957592dbf439426

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Jan 20, 2021

@criezy
Copy link
Member

@criezy criezy commented Jan 20, 2021

Just in case, I did a full md5 of the entire bc.exe, and it gives me 89db7324d34817972957592dbf439426

This matches the one I have.

Remind me what system you're running. Particularly if it's big endian, it could be indicative of some endian issues in the codebase.

I tried this on my new mac M1. It has an ARM chip, but it is little endian. I did some debugging and the AGS engine properly recognise it as being little endian. I am also compiling in 64 bits. I can try it on my intel mac in 32 and 64 bits to check if one of those works.

That being said the ASG code could be simplified to use the ScummVM endianness macros instead of rolling out its own code. Unless you want to keep the code as close as possible to the AGS code for now, I can give it a go and clean bbop.h.

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jan 20, 2021

The AGS engine does have some dodginess with casting pointers to numbers, at least as far as communicating with the plugin system is concerned, so there could be some effect on other parts as well. Last night I started looking into the error for Blackwell Legacy, but didn't get far. My current hypothesis is that because it has some of the data in a .001 file rather than just the executable, some of my I/O code is failing to currently set up the filename and load in the needed data. I'll try and verify & fix it tonight.

As for changing the codebase, minor changes like macro changes should be okay, but I do want to keep the codebase as close to the standalone version for as long as possible. I've been able to compile the standalone AGS interpreter project in Visual Studio, so it's very convenient for running the two side by side to track down where my library code isn't behaving as expected. With that in mind, I've already decided to put my work on getting rid of global variables on the backburner in favor of fixing errors with other games. That way, I'll be less likely to introduce new errors, since anything going wrong later on will be more easy to attribute to global changes rather than unimplemented library code.

@criezy
Copy link
Member

@criezy criezy commented Jan 20, 2021

I have now tested Black Cauldron on my Intel mac.

  • When ScummVM is compiled in 32-bit it works.
  • When ScummVM is compiled in 64-bit it doesn't work.

So this seems to confirm that this is a 32-bit vs 64bit issue.

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jan 21, 2021

Thanks for confirming the problem. It's something that will have to be dealt with. When I get a chance, hopefully the backtrace you provided may help narrow down where pointer casting is being done. Maybe I can introduce a union type of long and void * so pointer types in memory can be either 32-bits or 64-bits. Just depends on how widely such values are passed around. Worst case, the plugins system explicitly defines an intptr_t types as int64, and treats it the same for either numbers or pointers. Which is still bad, but at least safer, since pointers are at most 64 bits

@jdiperla
Copy link

@jdiperla jdiperla commented Jan 21, 2021

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Jan 22, 2021

One of the detected games is A Tale of Two Kingdoms. Is that the original or the deluxe edition? (I could be wrong, but I think the original version was freeware, while the deluxe edition is the one being sold.

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jan 22, 2021

Good question. I think it's the freeware version, but I'm not 100% sure. Back in the day I was one of the volunteer beta testers for the game, and ended up being the lead tester. So I still had a copy of the "final" version laying around in my backups, and just randomly picked it for testing as a more modern AGS game than Black Cauldron.

@criezy
Copy link
Member

@criezy criezy commented Jan 22, 2021

I assumed it was the Deluxe version, but I am not sure either. I have the Freeware version and I had to add a different detection entry for it (different md5 and it is also a bit smaller than the one you added). Here is part of my diff (I have more detection entries that I can push if you want).

@@ -46,8 +62,27 @@ static const PlainGameDescriptor GAME_NAMES[] = {
 
 static const AGSGameDescription GAME_DESCRIPTIONS[] = {
        ENGLISH_ENTRY("bcremake", "bc.exe", "0710e2ec71042617f565c01824f0cf3c", 7683255),
-       ENGLISH_ENTRY("atotk", "atotk.exe", "37cf2d4d07842d45b59c6dd9387c1ee7", 42872046),
+       ENGLISH_ENTRY("atotk", "atotk.exe", "37cf2d4d07842d45b59c6dd9387c1ee7", 42872046),    // Deluxe
+       ENGLISH_ENTRY("atotk", "atotk.exe", "37cf2d4d07842d45b59c6dd9387c1ee7", 42740200),    // Freeware
+       ENGLISH_ENTRY("kq1agdi", "kq1vga.exe", "688f1807c9d8df26fc0f174dc756054e", 8278611),  // 4.1c
        ENGLISH_ENTRY("kq2agdi", "kq2vga.exe", "40cfb7563df7dacf6530b19289a4745b", 12563246),   // 3.1
+       ENGLISH_ENTRY("kq2agdi", "kq2vga.exe", "40cfb7563df7dacf6530b19289a4745b", 12574643),  // 3.1c

That being said the sizes are actually quite close, so they might just be slightly different versions of the freeware version.
For the version I have, the executable creation date is July 20, 2007. However https://www.adventuregamestudio.co.uk/site/games/game/905-a-tale-of-two-kingdoms/ indicates a release date of July 16, so the original released version would have been a different version from the one I have if the dates are accurate.

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jan 22, 2021

Interesting. You're more than likely to be right, since I couldn't imagine the Deluxe version being so close in size. In any case, thanks for investigating, and providing detection entries. Every extra game/version detection will help in the long run.

@neuromancer
Copy link
Contributor

@neuromancer neuromancer commented Jan 22, 2021

Latest revision crashes when starting Primordia (from Steam):

Connected to Alsa sequencer client [128:0]
ALSA client initialized [129:0]
Initializing allegro

Initializing game data

WARNING: TODO: SetCurrentDirectory: ./!
Opened game data file: game28.dta

Game data version: 49

Compiled with: 3.5.0.5

Setting up game configuration

Data directory: ./

Voice pack found and initialized.

audio.vox found and initialized.

Setting up window

Initializing TTF renderer

Initializing keyboard

Initializing mouse: number of buttons reported is 0

Install timer

Sound settings: digital driver ID: 'AUTO' (0xffffffff), MIDI driver ID: 'AUTO' (0xffffffff)

Trying to init: digital driver ID: 'SCVM' (0x5343564d), MIDI driver ID: 'SCVM' (0x5343564d)

Installed digital driver ID: '' (0x1), MIDI driver ID: '' (0x1)

Install exit handler

Initialize legacy path finder library

Game GUI version: 119

Requested script API: v3.5.0 (6), compat level: v3.2.1 (0)

WARNING: font 'agsfnt0.wfn' has mistakes in data format, some characters may be displayed incorrectly

WARNING: font 'agsfnt3.wfn' has mistakes in data format, some characters may be displayed incorrectly

WARNING: font 'agsfnt5.wfn' has mistakes in data format, some characters may be displayed incorrectly

WARNING: font 'agsfnt16.wfn' has mistakes in data format, some characters may be displayed incorrectly

WARNING: font 'agsfnt19.wfn' has mistakes in data format, some characters may be displayed incorrectly

WARNING: font 'agsfnt23.wfn' has mistakes in data format, some characters may be displayed incorrectly

WARNING: font 'agsfnt26.wfn' has mistakes in data format, some characters may be displayed incorrectly

Plugin 'agsteam' could not be loaded (expected 'libagsteam.so'), trying built-in plugins...

Placeholder functions for the plugin 'agsteam' found.

Game title: 'Primordia'

Checking for disk space

Game native resolution: 320 x 200 (32 bit)

Graphic settings: driver: D3D9, windowed: no, screen def: max, screen size: 0 x 0, match device ratio: yes, game scale: proportional

Requested graphics driver 'D3D9' not found, will try existing drivers instead

Mouse control: off, base: 1.000000, speed: 1.000000

WARNING: TODO: video_on_gfxmode_changed!
Initialize sprites

Audio is processed on the main thread

Engine initialization complete

Starting game

WARNING: channel 2 - same clip assigned

scummvm: common/fs.cpp:112: Common::String Common::FSNode::getPath() const: Assertion `_realNode' failed.

Thread 1 "scummvm" received signal SIGABRT, Aborted.
0x00007ffff63fb615 in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007ffff63fb615 in raise () at /usr/lib/libc.so.6
#1  0x00007ffff63e4862 in abort () at /usr/lib/libc.so.6
#2  0x00007ffff63e4747 in _nl_load_domain.cold () at /usr/lib/libc.so.6
#3  0x00007ffff63f3bf6 in  () at /usr/lib/libc.so.6
#4  0x0000555555b576b6 in Common::FSNode::getPath() const (this=0x7fffffffd080) at common/fs.cpp:112
#5  0x0000555555b560c4 in Common::File::open(Common::FSNode const&) (this=0x55555682c310, node=...)
    at common/file.cpp:66
#6  0x000055555574be8e in AGS3::AGS::Shared::FileStream::Open(AGS3::AGS::Shared::String const&, AGS3::AGS::Shared::FileOpenMode, AGS3::AGS::Shared::FileWorkMode)
    (this=0x555556db2f80, file_name=..., open_mode=AGS3::AGS::Shared::kFile_Open, work_mode=AGS3::AGS::Shared::kFile_Read) at engines/ags/shared/util/filestream.cpp:185
#7  0x000055555574b674 in AGS3::AGS::Shared::FileStream::FileStream(AGS3::AGS::Shared::String const&, AGS3::AGS::Shared::FileOpenMode, AGS3::AGS::Shared::FileWorkMode, AGS3::AGS::Shared::DataEndianess)
    (this=0x555556db2f80, file_name=..., open_mode=AGS3::AGS::Shared::kFile_Open, work_mode=AGS3::AGS::Shared::kFile_Read, stream_endianess=AGS3::AGS::Shared::kLittleEndian) at engines/ags/shared/util/filestream.cpp:39
#8  0x0000555555835baa in AGS3::AGS::Shared::BufferedStream::BufferedStream(AGS3::AGS::Shared::String const&, AGS3::AGS::Shared::FileOpenMode, AGS3::AGS::Shared::FileWorkMode, AGS3::AGS::Shared::DataEndianess)
    (this=0x555556db2f80, file_name=..., open_mode=AGS3::AGS::Shared::kFile_Open, work_mode=AGS3::AGS::Shared::kFile_Read, stream_endianess=AGS3::AGS::Shared::kLittleEndian) at engines/ags/shared/util/bufferedstream.cpp:35
#9  0x0000555555761cc6 in AGS3::AGS::Shared::File::OpenFile(AGS3::AGS::Shared::String const&, AGS3::AGS::Shared::FileOpenMode, AGS3::AGS::Shared::FileWorkMode)
    (filename=..., open_mode=AGS3::AGS::Shared::kFile_Open, work_mode=AGS3::AGS::Shared::kFile_Read)
    at engines/ags/shared/util/file.cpp:120
#10 0x00005555557855cc in AGS3::FileOpen(char const*, AGS3::AGS::Shared::FileOpenMode, AGS3::AGS::Shared::FileWorkMode)
    (fnmm=0x555557098734 "$INSTALLDIR$/ENGV.tmp", open_mode=AGS3::AGS::Shared::kFile_Open, work_mode=AGS3::AGS::Shared::kFile_Read) at engines/ags/engine/ac/global_file.cpp:84
#11 0x0000555555841f3f in AGS3::sc_File::OpenFile(char const*, int)
    (this=0x5555582097b0, filename=0x555557098734 "$INSTALLDIR$/ENGV.tmp", mode=1)
    at engines/ags/engine/ac/dynobj/scriptfile.cpp:50
#12 0x000055555576916a in AGS3::sc_OpenFile(char const*, int)
    (fnmm=0x555557098734 "$INSTALLDIR$/ENGV.tmp", mode=1) at engines/ags/engine/ac/file.cpp:104
#13 0x000055555576ae48 in AGS3::Sc_sc_OpenFile(AGS3::RuntimeScriptValue const*, int)
    (params=0x7fffffffd8f0, param_count=2) at engines/ags/engine/ac/file.cpp:649
#14 0x00005555557f088b in AGS3::ccInstance::Run(int) (this=0x555557682b30, curpc=39722)
    at engines/ags/engine/script/cc_instance.cpp:1026
--Type <RET> for more, q to quit, c to continue without paging--
#15 0x00005555557ee432 in AGS3::ccInstance::CallScriptFunction(char const*, int, AGS3::RuntimeScriptValue const*)
    (this=0x555557682b30, funcname=0x555556158780 <AGS3::scfunctionname> "game_start", numargs=0, params=0x0)
    at engines/ags/engine/script/cc_instance.cpp:356
#16 0x00005555557f772f in AGS3::RunScriptFunctionIfExists(AGS3::ccInstance*, char const*, int, AGS3::RuntimeScriptValue const*)
    (sci=0x555557682b30, tsname=0x555556158780 <AGS3::scfunctionname> "game_start", numParam=0, params=0x0)
    at engines/ags/engine/script/script.cpp:402
#17 0x00005555557f78a9 in AGS3::RunTextScript(AGS3::ccInstance*, char const*)
    (sci=0x555557682b30, tsname=0x555555c5a4ba "game_start") at engines/ags/engine/script/script.cpp:448
#18 0x00005555557dc102 in AGS3::start_game() () at engines/ags/engine/main/game_start.cpp:100
#19 0x00005555557dc19e in AGS3::do_start_game() () at engines/ags/engine/main/game_start.cpp:123
#20 0x00005555557dc246 in AGS3::initialize_start_and_play_game(int, char const*)
    (override_start_room=0, loadSaveOnStartup=0x0) at engines/ags/engine/main/game_start.cpp:152
#21 0x00005555557d70db in AGS3::initialize_engine(AGS3::std::map<AGS3::AGS::Shared::String, AGS3::std::map<AGS3::AGS::Shared::String, AGS3::AGS::Shared::String, Common::Less<AGS3::AGS::Shared::String> >, Common::Less<AGS3::AGS::Shared::String> > const&) (startup_opts=...) at engines/ags/engine/main/engine.cpp:1468
#22 0x000055555575324f in AGS::AGSEngine::run() (this=0x555556d50a80) at engines/ags/ags.cpp:351
#23 0x000055555573312b in runGame(Plugin const*, OSystem&, Common::String const&)
    (plugin=0x5555565f2ad0, system=..., edebuglevels=...) at base/main.cpp:307
#24 0x0000555555734729 in scummvm_main(int, char const* const*) (argc=2, argv=0x7fffffffe6d8) at base/main.cpp:592
#25 0x00005555557305fd in main(int, char**) (argc=2, argv=0x7fffffffe6d8)
    at backends/platform/sdl/posix/posix-main.cpp:45
@dreammaster dreammaster force-pushed the dreammaster:ags branch 3 times, most recently from ab74437 to d3e9e80 Jan 25, 2021
@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Feb 3, 2021

If there no objections, I'll likely merge in the engine early Friday evening (PST) or Saturday morning. At this point, I've done test compiles under Unix, and fixed the bulk of the warnings; it all compiles fine.

The only thing left that I want to have done prior to the merge (and people starting to experiment) is to ensure there aren't any structure alignment issues with the savegames so that they remain compatible with the standalone interpreter, and we don't get any issues like save format differences between 32 and 64 bits. Anything else, like fixing remaining sprite rendering issues, plugin work, etc. can be done post-merge.

@sev-
Copy link
Member

@sev- sev- commented Feb 4, 2021

No objections from me

dreammaster added 23 commits Jan 31, 2021
When ScummVM is built with the new ENABLE_AGS_SCANNER define,
trying to start any game will instead scan all the folders
starting from the current directory downwards, and output
text suitable for pasting in the detection tables.
@dreammaster dreammaster force-pushed the dreammaster:ags branch from af227f3 to 19350d1 Feb 6, 2021
@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Feb 6, 2021

Stumbled on a low level string bug (opening the save dialog in KQ2) this evening during my final pre-merge testing. In case any fix for it involves playing around with the git history (such as if it's caused by my fix for 64-bit pointers), I'll wait to do the final merge tomorrow until I've had a chance to investigate it further.

@dreammaster dreammaster merged commit 515d592 into scummvm:master Feb 7, 2021
0 of 3 checks passed
0 of 3 checks passed
Codacy Static Code Analysis Codacy Static Code Analysis
Details
deepcode-ci-bot Found 14 warning issues, 7 minor issues.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants