ALG: Initial version of American Laser Games engine by dckone · Pull Request #6500 · scummvm/scummvm

dckone · 2025-03-22T10:50:22Z

Initial version of the American Laser Games engine.

Currently supported and can be completed:
Crime Patrol DOS
Crime Patrol Demo DOS
Drug Wars DOS
Drug Wars Demo DOS
Who Shot Johnny Rock? DOS
Mad Dog McCree DOS
Mad Dog II: The Lost Gold DOS
Space Pirates DOS
Space Pirates Demo DOS

Partially implemented, but unsupported and can probably not be completed
The Last Bounty Hunter DOS
The Last Bounty Hunter Demo DOS

So far only DOS versions are supported, in the future 3DO versions are quite possible.

Of the supported games, there are still some issues remaining:

Most original interpreters do not allow the cursor to go outside of the video area in most directions, this is unimplemented but minor.
Some minor timing differences in a few scenes.
Video decoder is not implemented similar to other video decoders and currently lives in engines/alg. Not sure if this is an issue as this video format is only used by ALG AFAIK.
Video fade-in is not implemented across the board, but it is not used that often and the effect is minor IMHO.
TODO: Weird pause at the beginning of MadDog2, probably a bug
Maybe some style issues in the code

Feedback would be much appreciated :)

P.S. Sorry for the squashed commit, the reason I did that is because so far I only commited to a local repo and only manually transferred to github infrequently, also my previous (local) commits did not follow the ScummVM commit rules yet.
Also there is quite a lot of refactoring going on in my local commits which makes them not very suitable for reviewing I guess.

engines/alg/game_spacepirates.cpp

dckone · 2025-03-22T13:51:41Z

I am aware there are still issues with memory leaks, I am trying to fix those in the next days

sev-

Solid work!

Checked 19 / 29 files, put my comments and suggestions. Unfortunately some renames are required.

engines/alg/alg.cpp

engines/alg/alg.h

engines/alg/configure.engine

sev- · 2025-03-22T14:53:06Z

engines/alg/game.cpp

+	_paletteDirty = true;
+	_screen = new Graphics::Surface();
+	_rnd = new Common::RandomSource("alg");
+	_screen->create(320, 200, Graphics::PixelFormat::createFormatCLUT8());


It is advised to not do anything heavy in the constructor. Please move these to Game::run()

@dckone the code is still in the init() function. Perhaps you forgot to push your changes?

@bluegr I do not understand. The code is in the init() method in Game, each inheriting game class then has a call to its specific init() method in ::run(), which in turn calls the parents init() method.

Is there a problem with that setup?

engines/alg/module.mk

engines/alg/scene.cpp

engines/alg/scene.h

engines/alg/video.cpp

bluegr · 2025-03-23T11:34:31Z

Since each game has its own hardcoded logic, it would make sense to group all the game logic in a folder. For example, instead of having game_maddog.cpp and game_maddog2.cpp, we could place all of these files in a logic folder, and end up with e.g. logic/maddog.cpp and logic/maddog2.cpp

dckone · 2025-03-23T12:32:21Z

Since each game has its own hardcoded logic, it would make sense to group all the game logic in a folder. For example, instead of having game_maddog.cpp and game_maddog2.cpp, we could place all of these files in a logic folder, and end up with e.g. logic/maddog.cpp and logic/maddog2.cpp

I like the idea, but not 100% sure how to achieve that at the moment. Currently each game has minor differences in handling its main game loop, as well as calling some functions provided in the base Game class.

The state of the game is held be the extended class, for example GameMaddog. And then the majority of the code is just functions called by the game script/Scene file. These again need access to the state.

So not 100% sure how to decouple all of this in a way that makes sense.

// EDIT: or do you mean just literally moving the game-specific files as-is to the subfolder? In that case that would be easy of course...

bluegr · 2025-03-23T12:50:42Z

// EDIT: or do you mean just literally moving the game-specific files as-is to the subfolder? In that case that would be easy of course...

Yes, I was asking for the game-specific files to be moved. Any further big refactorings in the code can be done at a later stage in-tree

dckone · 2025-03-23T13:26:47Z

Yes, I was asking for the game-specific files to be moved. Any further big refactorings in the code can be done at a later stage in-tree

ok, i moved it.

dckone · 2025-03-23T14:49:53Z

I have a question: Because my fork was already months behind the ScummVM master branch, I updated using the Github UI. Now the following can be seen in the PR commit history:

I was informed that might be a problem, as branch merges are not allowed.

How can I fix this? My git skills are not the best.

Also, is there a way to update my fork to parent master branch without creating such a problem?

lotharsm · 2025-03-23T18:13:12Z

Also, is there a way to update my fork to parent master branch without creating such a problem?

I can't reliably comment on how to fix the existing commit, because everytime I tried something like that, I horribly failed - so I'll leave this up to the others :-D

In order to prevent this to happen in the future, make sure that your git configuration is set to

git config pull.rebase true

This will avoid merge commits by always rebasing the current upstream code onto your fork.

Then, you can update your fork by running

git pull upstream master, assuming that you configured your repo to use github.com/scummvm/scummvm as the upstream remote. I don't think syncing by rebasing is possible via the GitHub UI...

bluegr · 2025-03-23T18:18:10Z

I have a question: Because my fork was already months behind the ScummVM master branch, I updated using the Github UI. Now the following can be seen in the PR commit history: I was informed that might be a problem, as branch merges are not allowed.

How can I fix this? My git skills are not the best.

Also, is there a way to update my fork to parent master branch without creating such a problem?

I could help with that. Essentially, the way to do this properly is to rebase your commits on top of the current ScummVM head. The problem is that this will require rebasing, which will rewrite your branch's history (via force pushing), since the commit hashes won't be the same anymore. If someone else does it for you, you'll have to reset your local branch with the changes from the remote branch. If you're OK with that, I can go ahead and fix the commit history for you.

dckone · 2025-03-24T02:35:42Z

I could help with that. Essentially, the way to do this properly is to rebase your commits on top of the current ScummVM head. The problem is that this will require rebasing, which will rewrite your branch's history (via force pushing), since the commit hashes won't be the same anymore. If someone else does it for you, you'll have to reset your local branch with the changes from the remote branch. If you're OK with that, I can go ahead and fix the commit history for you.

sounds great, I appreciate the help. I backed up my local repo just to be safe, and invited you as a collaborator on my github repo. I hope that is enough to give you write access.

sev-

Thank you for addressing the review feedback so quickly.

I have only few tiny notes, so I consider it ready for merge.

engines/alg/detection.h

engines/alg/detection_tables.h

sev- · 2025-03-24T03:30:43Z

engines/alg/alg.cpp

+bool AlgEngine::hasFeature(EngineFeature f) const {
+	return (f == kSupportsReturnToLauncher) ||
+		   (f == kSupportsLoadingDuringRuntime) ||
+		   (f == kSupportsSavingDuringRuntime);


You may also consider to add ExtendedSaves support here, so then screenshots, and other metadata is handled for you. But it could be added after the merge.

For now I would like to leave it as-is. There is only one save slot, as only saving via the original game interface is supported. Also screenshots might be misleading, as the save does not save exactly the location in the scene, but just the scene itself, so the scene is restarted on load, just like in the original interpreters.

engines/alg/game.cpp

engines/alg/graphics.cpp

engines/alg/logic/game_bountyhunter.cpp

engines/alg/logic/game_drugwars.cpp

bluegr · 2025-03-25T02:57:39Z

@dckone Fixed commit history

The commits are created by you, and committed by me (which is how git works)

dckone · 2025-03-25T05:28:31Z

@dckone Fixed commit history

The commits are created by you, and committed by me (which is how git works)

thank you very much for the help :)

engines/alg/alg.h

engines/alg/detection.h

engines/alg/detection_tables.h

engines/alg/detection.cpp

engines/alg/game.cpp

ccawley2011 · 2025-03-28T12:42:02Z

engines/alg/graphics.cpp

+	int32 dstX = x;
+	int32 dstY = y;
+	Common::Rect subRect = Common::Rect(0, 0, src->w, src->h);
+	if (dstX < 0) {


There's a clip function in Graphics::Surface that may help simplify this.

i will look into that hopefully soon

after trying for 2 hours i gave up, somehow I do not understand how this clip function could work in my case. I might look into it again if I have more time in the future.

ccawley2011 · 2025-03-28T12:47:31Z

engines/alg/graphics.cpp

+				for (uint16 x = 0; x < width; x++) {
+					aniImage->setPixel(x, y, aniFile.readByte());
+				}


Suggested change

for (uint16 x = 0; x < width; x++) {

aniImage->setPixel(x, y, aniFile.readByte());

}

aniFile.read(aniImage->getBasePtr(0, y), width);

Something similar can also be done in other places that read in rows of pixels at a time.

same as above, will look into it

ccawley2011 · 2025-03-28T12:49:50Z

engines/alg/graphics.cpp

+	surface->create(width, height, Graphics::PixelFormat::createFormatCLUT8());
+	uint8 *pixels = new uint8[width * height]();
+	vgaFile.read(pixels, width * height);
+	surface->setPixels(pixels);


This leaks the memory allocated when calling surface->create. It might be simpler to just read directly to the surface memory rather than creating a separate buffer for it.

do I interpret that correctly that the "new Graphics::Surface()" already allocates a pixel buffer and then the create() allocates another one and the original one is not freed?

Close - create() creates the first buffer, then setPixels replaces it with the manually allocated one without freeing the first one.

should be fixed now

ccawley2011 · 2025-03-28T12:57:15Z

engines/alg/video.cpp

+		}
+		while (runLength > 0) {
+			if (color > 0) {
+				_frame->setPixel(x, y, color);


You may find memset useful here for performance with longer values of runLength.

will look into it

ccawley2011 · 2025-03-28T13:00:05Z

engines/alg/logic/game_bountyhunter.cpp

+			error("GameBountyHunter::run(): Cannot find scene %s in libfile", scene->_name.c_str());
+		}
+		_sceneSkipped = false;
+		Audio::PacketizedAudioStream *audioStream = _videoDecoder->getAudioStream();


I'd recommend keeping the implementation details for the video decoder self-contained in case you need to support alternative video formats later.

I am not sure what you mean by that. Is it that I should let the videoDecoder handle the playback of the audio?

Basically, yes.

i moved the audio stream handling into the video decoder as a quick fix.

In the future I might rework the video decoding, currently it is a bit hacky in places. Also the frame timing should be handled by the video decoder, currently it is handled by game logic.

ccawley2011 · 2025-03-28T15:55:11Z

Would it be possible to provide the demos listed in the detection tables on our demos page?

dckone · 2025-03-28T16:09:19Z

Would it be possible to provide the demos listed in the detection tables on our demos page?

Sure, I need to request access, then I can supply these. I will followup on that soon hopefully.
Same problem with some of the previously mentioned issues, starting tomorrow I will be on vacation for 2 weeks. Hopefully I can sneak in some coding on these issues, if not I will definitely work on it once back.

engines/alg/game.cpp

bluegr · 2025-03-29T02:24:12Z

engines/alg/alg.cpp

+	case Alg::GType_CRIME_PATROL: {
+		GameCrimePatrol *game = new GameCrimePatrol(this, gd);
+		_debugger = new DebuggerCrimePatrol(game);
+		_game = game;
+		break;
+	}
+	case Alg::GType_DRUG_WARS: {
+		GameDrugWars *game = new GameDrugWars(this, gd);
+		_debugger = new DebuggerDrugWars(game);
+		_game = game;
+		break;
+	}


Suggested change

case Alg::GType_CRIME_PATROL: {

GameCrimePatrol *game = new GameCrimePatrol(this, gd);

_debugger = new DebuggerCrimePatrol(game);

_game = game;

break;

}

case Alg::GType_DRUG_WARS: {

GameDrugWars *game = new GameDrugWars(this, gd);

_debugger = new DebuggerDrugWars(game);

_game = game;

break;

}

case Alg::GType_CRIME_PATROL: {

_game = new GameCrimePatrol(this, gd);

_debugger = new DebuggerCrimePatrol(_game);

break;

}

case Alg::GType_DRUG_WARS: {

_game = new GameDrugWars(this, gd);

_debugger = new DebuggerDrugWars(_game);

break;

}

Same for the cases below

If i implement that change it does not compile anymore:

engines/alg/alg.cpp: In constructor 'Alg::AlgEngine::AlgEngine(OSystem*, const Alg::AlgGameDescription*)':
engines/alg/alg.cpp:43:53: error: invalid conversion from 'Alg::Game*' to 'Alg::GameCrimePatrol*' [-fpermissive]
43 | _debugger = new DebuggerCrimePatrol(_game);

engines/alg/detection.cpp

engines/alg/game.cpp

sev- · 2025-04-10T20:50:12Z

Thanks, merging

dckone changed the title ~~ALG: Initial version~~ DRAFT: ALG: Initial version Mar 22, 2025

bluegr reviewed Mar 22, 2025

View reviewed changes

engines/alg/game_spacepirates.cpp Outdated Show resolved Hide resolved

bluegr reviewed Mar 22, 2025

View reviewed changes

engines/alg/game_spacepirates.cpp Outdated Show resolved Hide resolved

dckone changed the title ~~DRAFT: ALG: Initial version~~ ALG: Initial version of American Laser Games engine Mar 22, 2025

sev- requested changes Mar 22, 2025

View reviewed changes

dckone requested a review from sev- March 23, 2025 10:41

dckone force-pushed the master branch from 05001a5 to f11d856 Compare March 23, 2025 13:25

sev- approved these changes Mar 24, 2025

View reviewed changes

loki added 6 commits March 25, 2025 04:54

ALG: Initial version

aff56e1

ALG: fix MSVC errors and warnings

c30ef9c

ALG: formatting, code style, implement feedback

899dc45

ALG: Graphics: fix unnecessary copying of surfaces on load

6a7526d

ALG: moved game-specific files to logic sub-folder

fbfb593

ALG: fix a few memory leaks (hopefully)

f7afff3

bluegr force-pushed the master branch from 0e3513c to f7afff3 Compare March 25, 2025 02:55

ALG: Refactoring, feedback implemented, bug fixes

319d7d8

ccawley2011 reviewed Mar 28, 2025

View reviewed changes

ALG: Implement some fixes from review

51126dc

ccawley2011 reviewed Mar 28, 2025

View reviewed changes

engines/alg/game.cpp Outdated Show resolved Hide resolved

bluegr reviewed Mar 29, 2025

View reviewed changes

engines/alg/detection.cpp Outdated Show resolved Hide resolved

bluegr reviewed Mar 29, 2025

View reviewed changes

engines/alg/detection.cpp Show resolved Hide resolved

bluegr reviewed Mar 29, 2025

View reviewed changes

engines/alg/detection.cpp Outdated Show resolved Hide resolved

bluegr reviewed Mar 29, 2025

View reviewed changes

engines/alg/game.cpp Outdated Show resolved Hide resolved

loki added 2 commits April 9, 2025 00:48

ALG: refactoring from PR feedback

19baf97

ALG: Video and graphics refactoring

8ef8e79

sev- merged commit 3e03257 into scummvm:master Apr 10, 2025
8 checks passed

Uh oh!

Conversation

dckone commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dckone commented Mar 22, 2025

Uh oh!

sev- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluegr commented Mar 23, 2025

Uh oh!

dckone commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluegr commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dckone commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dckone commented Mar 23, 2025

Uh oh!

lotharsm commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluegr commented Mar 23, 2025

Uh oh!

dckone commented Mar 24, 2025

Uh oh!

sev- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluegr commented Mar 25, 2025

Uh oh!

dckone commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dckone commented Mar 22, 2025 •

edited

Loading

dckone commented Mar 23, 2025 •

edited

Loading

bluegr commented Mar 23, 2025 •

edited

Loading

dckone commented Mar 23, 2025 •

edited

Loading

lotharsm commented Mar 23, 2025 •

edited

Loading

dckone commented Mar 28, 2025 •

edited

Loading