AGS: AGS engine for ScummVM #2738
Conversation
DeepCode's analysis on #c0abf0 found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
|
Wheee, awesome, can't wait to test my AGS library of games |
|
I tried a few games, and none of them is working. Here are the various errors I am getting:
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. |
|
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 |
|
* Gemini Rue, Kathy Rain, Quest for Infamy: |Error: TODO: Handle 32-bit pointers in ScummVM if needed!|
According to https://steamcommunity.com/app/370910/discussions/0/361787186424501858/ the Kathy Rain developer (developers?) used his own fork of AGS. It's unclear to me how much it actually differs from the official version, because he went on to say that "All of my engine fixes have either been added in later versions of AGS, or were hand picked from various open branches, like Draconian edition, the Wadjet Eye fork etc."
So maybe he just wanted to protect himself from future AGS versions breaking the game? I do remember having some problems with some of Czho mythos games when using AGS 2.7.2 instead of AGS 2.6.2, but I think later AGS 3 versions fixed those incompatibilities.
Apparently his version lives at https://github.com/josthas/KrusAGS and he also has a WoaMAGS repository for Whispers of a Machine. I haven't looked for the other forks he referred to.
(Fun Fact: The then official Linux version of AGS accidentally swapped the red and blue colour components in some games, e.g. Trilby's Notes. It made the game look very surreal, but in some ways actualy more atmospheric. :-)
|
This matches the one I have.
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 |
|
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. |
|
I have now tested Black Cauldron on my Intel mac.
So this seems to confirm that this is a 32-bit vs 64bit issue. |
|
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 |
|
I think most of this has been done in the new AGS repository. They just did
a complete refactoring of the code.
https://github.com/adventuregamestudio/ags/tree/ags3--sdl2
https://www.adventuregamestudio.co.uk/forums/index.php?topic=58636.0
…On Wed, Jan 20, 2021, 7:31 PM Paul Gilbert ***@***.***> wrote:
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
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2738 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOXBBZSFKS3O3GCVPHTPILS25YWDANCNFSM4WHUIE6A>
.
|
|
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. |
|
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. |
|
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.1cThat being said the sizes are actually quite close, so they might just be slightly different versions of the freeware version. |
|
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. |
|
Latest revision crashes when starting Primordia (from Steam): |
dreammaster
force-pushed
the
ags
branch
3 times, most recently
from
ab74437
to
d3e9e80
Compare
|
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. |
|
No objections from me |
Based on AGS 3.5.0.27
|
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. |
that's bollocks, strncpy is inefficient and unsafe. the modern portable and safe way to do a bounded strcpy is posix manpage for strncpy: https://man7.org/linux/man-pages/man3/strncpy.3p.html |
|
@rofl0r : I assume you are responding to #2738 (comment) which is a comment from deepcode-ci-bot. As per the name, it is a bot and thus will not read your comments. You might want to send a PR to deepcode about that. |
right, but i also assumed that this was addressed by the PR author (using the proposed bogus solution) since he force-pushed the branch after said comment. |
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: