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

ENGINES: Add an engine id #1210

Merged
merged 12 commits into from
Nov 3, 2019
Merged

ENGINES: Add an engine id #1210

merged 12 commits into from
Nov 3, 2019

Conversation

bgK
Copy link
Member

@bgK bgK commented Jun 3, 2018

Currently games are uniquely identified by their gameId. This is good because a single value can be used to identify a game across all the engines. The gameId is stored in the configuration file under the game target section.

However, as more game engines have been added to ScummVM, the concern of gameId clashes across engines became apparent. A feature was added to the Advanced Detector to work around this issue: singleId. For engines that enable the singleId feature, the gameId is always the same for all the games supported by the engine, allowing to avoid collisions.
However, storing the singleId instead of the gameId in the configuration file is a loss of information that causes a few problems:

  • The --list-games command line command no longer lists all the games supported by the engines using singleId.
  • When multiple games using the same engine are stored in the same folder, ScummVM doesn't know which one to launch, and starts the first one (This is annoying for Myst because both the game and the making of are in the same folder on the game CD).
  • In the MetaEngines, in code that is executed before detection happens, it's not possible to know which game the currently active target is for. This is a problem for the Mohawk engine because saved game handling is entirely separate across the games. Currently we have to rely on the target name to detect the game (
    if (strstr(target, "myst")) {
    ). This is not robust because the target name is user configurable and could very well not contain the game name.

In this PR, I propose to use a different approach. Engines have a new engineId property that can be used to uniquely identify them. That way, the gameIds for an engine are namespaced by the engineId. This removes the need for the singleId feature which I propose to remove. The engineIds are stored in the configuration file under the game targets sections along with the gameIds.

This pull request contains code allowing to automatically upgrade the game targets to add an engineId and set the appropriate gameId when trying to launch a game (and other operations that require an active game target context such as editing the game options or loading a saved game).

On the command line, I have added the ability to use qualified game names where gameIds were previously used. Qualified game names have the following form: engineId:gameId. It is now recommended to say:
scummvm --path=/my/games/riven mohawk:riven instead of scummvm --path=/my/games/riven riven.
Non-qualified game names still work as long as there are no gameId conflicts but are deprecated. (They are deprecated because looking up a game by its gameId only has become a slow path when using dynamic plugins. This is because the gameId => plugin path cache stored in the configuration file has become a engineId => plugin path cache to allow the now more common operation of looking up an engine by its name to be a fast path.)

On the command line, I have also added a new --list-engines command allowing to print the list of compiled engines along with their ids.

Please review.

@@ -1143,9 +1144,15 @@ void upgradeTargets() {
// ScummVM still generates an incorrect description string. So, the description
// should only be updated if the user explicitly requests this.
#if 0
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Oops, this looks like it needs resolving :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed!

if (idCameFromCommandLine)
ConfMan.set("id_came_from_command_line", "1");

ConfMan.set("id_came_from_command_line", "1");
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, this workaround is still semi-broken (see bug #2788).

Copy link
Member Author

Choose a reason for hiding this comment

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

I have pushed a fix for bug #2788 to master since it is a simple correction.

@bonki
Copy link
Member

bonki commented Jun 3, 2018

Without understanding how the current system exactly works and not having looked at the code in detail yet your explanation makes total sense to me and is in my opinion a desirable change.

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.

The idea is great.

However, it is not quite clear why you're also changing the single game engine names into names of those games (besides Cryo and Xeen, where there are the inconsistencies). Also, I am worried a bit about the obsolete target upgrading. I am not sure it still works.

In general, the code looks clean, but I would like to test it before releasing, especially for it working well with non-upgraded config files.

{"fw", "Future Wars"},
{"os", "Operation Stealth"},
{0, 0}
};

static const Engines::ObsoleteGameID obsoleteGameIDsTable[] = {
Copy link
Member

Choose a reason for hiding this comment

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

Why? Will it still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have removed the obsolete engine id tables in the cases where the old obsolete gameIds are the new migrated game ids. In these cases the engineId will be found by the new target upgrade code using the gameId.

}

virtual const char *getOriginalCopyright() const {
return "Cruise for a Corpse (C) Delphine Software";
return "Cinematique evo 2 (C) Delphine Software";
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Above you were changing Chewy to the game name, and here you're doing the opposite. Is there some reasoning behind this inconsistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it's to be consistent with the cine engine.

}

virtual const char *getOriginalCopyright() const {
return "Cryo Engine (C) Cryo Interactive";
return "Cryo (C) Cryo Interactive";
Copy link
Member

Choose a reason for hiding this comment

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

That makes no sense. It should stay "Cryo Engine". Or, if you want to put the game name, then it is "Lost Eden".

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

_maxScanDepth = 2;
_directoryGlobs = directoryGlobs;
}

virtual const char *getEngineId() const {
Copy link
Member

Choose a reason for hiding this comment

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

It is virtual here, but not declared virtual in the previous instances. Maybe it makes sense to normalize it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather do mass change using clang-tidy in another pull request to add the override attributes / remove the virtual attributes rather than hand editing all these files again.

const PluginList &getPlugins() const;

/** Find a target */ // TODO: Expand on description
Copy link
Member

Choose a reason for hiding this comment

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

Please, close this TODO ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}

virtual const char *getOriginalCopyright() const {
return "Xeen Engine (c) 1992-1993 New World Computing, Inc.";
return "Xeen (c) 1992-1993 New World Computing, Inc.";
Copy link
Member

Choose a reason for hiding this comment

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

The engine supports not just one game.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

@bgK
Copy link
Member Author

bgK commented Jun 3, 2018

However, it is not quite clear why you're also changing the single game engine names into names of those games (besides Cryo and Xeen, where there are the inconsistencies).

This is to make the output of the --list-engines command line option look better (IMO).

With the game engines renaming:

Engine ID       Engine Name                                           
--------------- ------------------------------------------------------
access          Access
adl             ADL
agi             AGI preAGI + v2 + v3
agos            AGOS
avalanche       Avalanche
bbvs            MTV's Beavis and Butt-head in Virtual Stupidity
bladerunner     Blade Runner
cge             CGE
cge2            CGE2
chewy           Chewy: Esc from F5
cine            Cinematique evo 1
composer        Magic Composer
cruise          Cinematique evo 2
cryo            Cryo
director        Macromedia Director
dm              Dungeon Master
draci           Draci Historie
drascula        Drascula: The Vampire Strikes Back
dreamweb        DreamWeb
fullpipe        Full Pipe
gnap            Gnap
gob             Gob
groovie         Groovie
hopkins         Hopkins FBI
hugo            Hugo
kyra            Kyra
lab             Labyrinth of Time
lastexpress     The Last Express
lilliput        Lilliput
lure            Lure of the Temptress
macventure      MacVenture
made            MADE
mads            MADS
mohawk          Mohawk
mortevielle     Mortville Manor
neverhood       The Neverhood Chronicles
parallaction    Parallaction
pegasus         The Journeyman Project: Pegasus Prime
plumbers        Plumbers Don't Wear Ties
prince          The Prince and the Coward
queen           Flight of the Amazon Queen
saga            SAGA [all games]
sci             SCI [all games]
scumm           SCUMM [all games]
sherlock        Sherlock
sky             Beneath a Steel Sky
sludge          Sludge
supernova       Mission Supernova
sword1          Broken Sword: The Shadow of the Templars
sword2          Broken Sword II: The Smoking Mirror
sword25         Broken Sword 2.5
teenagent       TeenAgent
testbed         TestBed: The Backend Testing Framework
tinsel          Tinsel
titanic         Starship Titanic
toltecs         3 Skulls of the Toltecs
tony            Tony Tough and the Night of Roasted Moths
toon            Toonstruck
touche          Touche: The Adventures of the Fifth Musketeer
tsage           TsAGE
tucker          Bud Tucker in Double Trouble
voyeur          Voyeur
wage            World Adventure Game Engine
wintermute      Wintermute
xeen            Xeen

Without the game engine renaming:

Engine ID       Engine Name                                           
--------------- ------------------------------------------------------
access          Access Engine
adl             ADL
agi             AGI preAGI + v2 + v3
agos            AGOS
avalanche       Avalanche
bbvs            MTV's Beavis and Butt-head in Virtual Stupidity
bladerunner     Blade Runner Engine
cge             CGE
cge2            CGE2
chewy           Chewy Engine
cine            Cine
composer        Magic Composer Engine
cruise          CruisE
cryo            Cryo Engine
director        Macromedia Director
dm              Dungeon Master
draci           Draci
drascula        Drascula
dreamweb        DreamWeb engine
fullpipe        Fullpipe Engine
gnap            Gnap
gob             Gob
groovie         Groovie
hopkins         Hopkins Engine
hugo            Hugo
kyra            Kyra
lab             Lab
lastexpress     Lastexpress
lilliput        Lilliput
lure            Lure
macventure      MacVenture
made            MADE
mads            MADS Engine
mohawk          Mohawk
mortevielle     Mortevielle
neverhood       Neverhood Engine
parallaction    Parallaction
pegasus         The Journeyman Project: Pegasus Prime
plumbers        Plumbers Don't Wear Ties' Engine
prince          Prince Engine
queen           Queen
saga            SAGA [all games]
sci             SCI [SCI0, SCI01, SCI10, SCI11, SCI32]
scumm           SCUMM [all games]
sherlock        Sherlock Engine
sky             Sky
sludge          Sludge Engine
supernova       Supernova Engine
sword1          Sword1
sword2          Sword2
sword25         Sword25
teenagent       TeenAgent
testbed         TestBed: The Backend Testing Framework
tinsel          Tinsel
titanic         Titanic Engine
toltecs         Toltecs Engine
tony            Tony Engine
toon            Toon
touche          Touche
tsage           TsAGE
tucker          Tucker
voyeur          Voyeur Engine
wage            World Adventure Game Engine
wintermute      Wintermute
xeen            Xeen Engine

I don't care too much about that change and would not mind reverting it.

@sev-
Copy link
Member

sev- commented Jun 3, 2018

Ah, OK. Now I see where the uniformity comes from and it makes sense.

@dafioram
Copy link
Contributor

  1. How would you feel about beneath a steel sky being sky:bass? It looks like you have a convention of a single game has the same name as the engine so if you want to keep that convention that makes sense.

  2. When I use --list-engines should sword25 appear after sword2 games (it was previously listed after)?

  3. If I run mass add on a directory that has 55 valid scummvm games and 6 non-scummvm folders it finds about 23-35 of them. It then crashes on Titanic/German, Riven/CD, lighthouse, Zork CD, Shivers, others. I ran it a bunch of times removing the bad folder/games. I'm not positive that its is a particular game that it crashes because of when I remove the game it crashed on it will often crash again on the game that it searches before that game, but it previously found fine before. This was done on a clean scummvm.ini. Adding the games individual works just fine. The crash is shown below

If I do the same thing with the current master It finds 55 games (It reported some duplicates also for Zork CD, and Labyrinth of Time). It may have missed a game as there are lots of different languages added as different games so maybe I miss counted.

#0 0x0000555557a24a2c in Common::SharedPtr::operator_bool() const (this=0x8) at ./common/ptr.h:166
#1 0x0000555557a248c8 in Common::SafeBool<Common::SharedPtr, Common::impl::no_base<Common::SharedPtr > >::operator Common::SharedPtr* Common::impl::safe_bool_impl<Common::SharedPtr >::() const (this=0x8)
at ./common/safe-bool.h:54
#2 0x0000555557a2339a in Common::FSNode::getParent() const (this=0x0) at common/fs.cpp:100
#3 0x00005555559b751c in EngineManager::detectGames(Common::FSList const&) const (this=0x555559759380, fslist=...) at base/plugins.cpp:510
#4 0x00005555577f7f3c in GUI::MassAddDialog::handleTickle() (this=0x7ffffffb7c00) at gui/massadd.cpp:187
#5 0x00005555577f17a4 in GUI::GuiManager::runLoop() (this=0x555559290480) at gui/gui-manager.cpp:327
#6 0x00005555577ea642 in GUI::Dialog::runModal() (this=0x7ffffffb7c00) at gui/dialog.cpp:80
#7 0x00005555577f48ff in GUI::LauncherDialog::addGame() (this=0x7ffffffb8500) at gui/launcher.cpp:316
#8 0x00005555577f5cbf in GUI::LauncherDialog::handleCommand(GUI::CommandSender
, unsigned int, unsigned int) (this=0x7ffffffb8500, sender=0x55555966b058, cmd=1094992967, data=0) at gui/launcher.cpp:599
#9 0x000055555707783d in GUI::CommandSender::sendCommand(unsigned int, unsigned int) (this=0x55555966b058, cmd=1094992967, data=0)
at ./gui/object.h:55
#10 0x0000555557832474 in GUI::ButtonWidget::handleMouseUp(int, int, int, int) (this=0x55555966af90, x=58, y=9, button=1, clickCount=1)
at gui/widget.cpp:332
#11 0x00005555577eacc4 in GUI::Dialog::handleMouseUp(int, int, int, int) (this=0x7ffffffb8500, x=567, y=192, button=1, clickCount=1)
at gui/dialog.cpp:226
#12 0x00005555577f24a2 in GUI::GuiManager::processEvent(Common::Event const&, GUI::Dialog*) (this=0x555559290480, event=..., activeDialog=0x7ffffffb8500) at gui/gui-manager.cpp:588
#13 0x00005555577f1866 in GUI::GuiManager::runLoop() (this=0x555559290480) at gui/gui-manager.cpp:359
#14 0x00005555577ea642 in GUI::Dialog::runModal() (this=0x7ffffffb8500) at gui/dialog.cpp:80
#15 0x00005555559a3c02 in launcherDialog() () at base/main.cpp:106
#16 0x00005555559a5976 in scummvm_main(int, char const* const*) (argc=1, argv=0x7fffffffdfb8) at base/main.cpp:515
#17 0x00005555559a2b6c in main(int, char**) (argc=1, argv=0x7fffffffdfb8) at backends/platform/sdl/posix/posix-main.cpp:45

@bgK
Copy link
Member Author

bgK commented Jun 17, 2018

  1. How would you feel about beneath a steel sky being sky:bass?

I feel like this is a bit outside of the scope of this PR. Especially since BASS was not using the singleId system. It can be changed later using the obsoleteId mechanism.

  1. When I use --list-engines should sword25 appear after sword2 games (it was previously listed after)?

I'm not sure what you mean. sword25 appears after the other two sword games in the example output I posted above.

  1. If I run mass add on a directory that has 55 valid scummvm games and 6 non-scummvm folders it finds about 23-35 of them. It then crashes.

I've pushed a fix for this to master. It was introduced in one of my previous pull requests.

@dafioram
Copy link
Contributor

For 2. I get a different ordering for --list-games vs. --list-engines

When I run ./scummvm --list-games I get the following for the sword games:
sword1:sword1 Broken Sword: The Shadow of the Templars
sword1:sword1demo Broken Sword: The Shadow of the Templars (Demo)
sword1:sword1mac Broken Sword: The Shadow of the Templars (Mac)
sword1:sword1macdemo Broken Sword: The Shadow of the Templars (Mac demo)
sword1:sword1psx Broken Sword: The Shadow of the Templars (PlayStation)
sword1:sword1psxdemo Broken Sword: The Shadow of the Templars (PlayStation demo)
sword25:sword25 Broken Sword 2.5
sword2:sword2 Broken Sword II: The Smoking Mirror
sword2:sword2alt Broken Sword II: The Smoking Mirror (alt)
sword2:sword2demo Broken Sword II: The Smoking Mirror (Demo)
sword2:sword2psx Broken Sword II: The Smoking Mirror (PlayStation)
sword2:sword2psxdemo Broken Sword II: The Smoking Mirror (PlayStation/Demo)

so I am seeing sword25 before sword2

For 3.
That fixed it. I am now able to run mass add as it completes detecting all the same games as before.

If however I run mass add again on the same directory I get another crash.

#0 0x00007ffff40c8e97 in __GI_raise (sig=sig@entry=6)
at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007ffff40ca801 in __GI_abort () at abort.c:79
#2 0x00007ffff40ba39a in __assert_fail_base (fmt=0x7ffff42417d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x555557fbfb33 "x != nullptr", file=file@entry=0x555557fbfac0 "common/str.cpp", line=line@entry=650, function=function@entry=0x555557fbffe0 <Common::String::operator==(char const*) const::PRETTY_FUNCTION> "bool Common::String::operator==(const char*) const")
at assert.c:92
#3 0x00007ffff40ba412 in __GI___assert_fail (assertion=0x555557fbfb33 "x != nullptr", file=0x555557fbfac0 "common/str.cpp", line=650, function=0x555557fbffe0 <Common::String::operator==(char const*) const::PRETTY_FUNCTION> "bool Common::String::operator==(const char*) const") at assert.c:101
#4 0x0000555557a41f28 in Common::String::operator==(char const*) const (this=0x555558d7cf48, x=0x0) at common/str.cpp:650
#5 0x00005555577fc336 in GUI::MassAddDialog::handleTickle() (this=0x7ffffffb7c00) at gui/massadd.cpp:224
#6 0x00005555577f585c in GUI::GuiManager::runLoop() (this=0x555559301430)
at gui/gui-manager.cpp:327
#7 0x00005555577ee6fa in GUI::Dialog::runModal() (this=0x7ffffffb7c00)
at gui/dialog.cpp:80
#8 0x00005555577f89b7 in GUI::LauncherDialog::addGame() (this=0x7ffffffb8500)
at gui/launcher.cpp:316
#9 0x00005555577f9d77 in GUI::LauncherDialog::handleCommand(GUI::CommandSender*, unsigned int, unsigned int) (this=0x7ffffffb8500, sender=0x5555596d5548, cmd=1094992967, data=0) at gui/launcher.cpp:599
#10 0x000055555707b8f5 in GUI::CommandSender::sendCommand(unsigned int, unsigned int) (this=0x5555596d5548, cmd=1094992967, data=0) at ./gui/object.h:55
#11 0x000055555783652c in GUI::ButtonWidget::handleMouseUp(int, int, int, int) (this=0x5555596d5480, x=86, y=6, button=1, clickCount=1) at gui/widget.cpp:332
#12 0x00005555577eed7c in GUI::Dialog::handleMouseUp(int, int, int, int) (this=0x7ffffffb8500, x=595, y=189, button=1, clickCount=1) at gui/dialog.cpp:226
#13 0x00005555577f655a in GUI::GuiManager::processEvent(Common::Event const&, GUI::Dialog*) (this=0x555559301430, event=..., activeDialog=0x7ffffffb8500)
at gui/gui-manager.cpp:588
#14 0x00005555577f591e in GUI::GuiManager::runLoop() (this=0x555559301430)
at gui/gui-manager.cpp:359
#15 0x00005555577ee6fa in GUI::Dialog::runModal() (this=0x7ffffffb8500)
at gui/dialog.cpp:80
#16 0x00005555559a49b2 in launcherDialog() () at base/main.cpp:106
#17 0x00005555559a6726 in scummvm_main(int, char const* const*) (argc=1, argv=0x7fffffffdfb8) at base/main.cpp:512
#18 0x00005555559a391c in main(int, char**) (argc=1, argv=0x7fffffffdfb8)
at backends/platform/sdl/posix/posix-main.cpp:45

@bgK
Copy link
Member Author

bgK commented Jun 30, 2018

If however I run mass add again on the same directory I get another crash.

Great catch, I've pushed a fix for this to master. It was introduced in one of my previous pull requests.

@dafioram
Copy link
Contributor

dafioram commented Jun 30, 2018

I still have the crash if I use this branch. If I merge this branch into master I hit issues with pink when compiling.

In file included from ./engines/metaengine.h:33:0,
                 from ./engines/advancedDetector.h:26,
                 from ./gui/EventRecorder.h:32,
                 from engines/pink/detection.cpp:23:
engines/pink/detection.cpp: In function ‘PluginObject* g_PINK_getObject()’:
./base/plugins.h:116:26: error: invalid new-expression of abstract class type ‘PinkMetaEngine’
   return new PLUGINCLASS(); \
                          ^
engines/pink/detection.cpp:130:1: note: in expansion of macro ‘REGISTER_PLUGIN_STATIC’
 REGISTER_PLUGIN_STATIC(PINK, PLUGIN_TYPE_ENGINE, PinkMetaEngine);
 ^~~~~~~~~~~~~~~~~~~~~~
engines/pink/detection.cpp:41:7: note:   because the following virtual functions are pure within ‘PinkMetaEngine’:
 class PinkMetaEngine : public AdvancedMetaEngine {
       ^~~~~~~~~~~~~~
In file included from ./engines/advancedDetector.h:26:0,
                 from ./gui/EventRecorder.h:32,
                 from engines/pink/detection.cpp:23:
./engines/metaengine.h:69:22: note: 	virtual const char* MetaEngine::getEngineId() const
  virtual const char *getEngineId() const = 0;

@bgK
Copy link
Member Author

bgK commented Jun 30, 2018

I still have the crash if I use this branch. If I merge this branch into master I hit issues with pink when compiling.

Okay, I've updated this branch with master so you don't have to.

I get a different ordering for --list-games vs. --list-engines

I don't care too much about the order in those lists, so I have removed the custom sort. The games now appear in the same order as everywhere else.

@dafioram
Copy link
Contributor

This looks good.

@dafioram
Copy link
Contributor

Do we want to plan a date that this PR is to get merged so devs can get ready?

@dafioram
Copy link
Contributor

Plans to merge this in?

@lotharsm
Copy link
Member

lotharsm commented Dec 4, 2018

Any updates on this? Would be a shame to see this PR going bitrotting.

@bgK
Copy link
Member Author

bgK commented Sep 30, 2019

I'll spend some time testing this again to make sure it has not bit-rotten too badly and merge it soon after 2.1.0 is released, if nobody disagrees.

The engine ID identifies which engine should be used to launch the target.
Also remove the 'single ID' system. Different games from engines that used
that system now have different game IDs.

Also-By: Matthew Hoops <clone2727@gmail.com>
The target is user defined and not may not contain the game name
…names

Qualified game names have the following form: engineId:gameId.
Unqualified game names are still supported as long as they are not
ambiguous. However they are considered deprecated and are no longer
displayed by the --list-games command.
@bgK bgK merged commit 6919e71 into scummvm:master Nov 3, 2019
@bgK bgK deleted the add-engineid5 branch November 3, 2019 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants