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: ALL: Split MetaEngines into 2 parts, statically link detection features across all engines. #2403

Merged
merged 200 commits into from Oct 3, 2020

Conversation

@aryanrawlani28
Copy link
Contributor

@aryanrawlani28 aryanrawlani28 commented Aug 12, 2020

Updates:

  • MetaEngine is now renamed to MetaEngineStatic (has static parts - detection, etc)
  • MetaEngineConnect is now renamed to MetaEngine (has dynamic parts)
  • Wintermute & Sci fallback detection disabled by default for systems which use UncachedPluginManager

Known issues:

  • File structuring issues for Visual Studio.
  • Dynamic detection plugin not loading for some systems, needs some location reworking.

This PR splits apart the MetaEngines into a way that detection features are always available* to the executable.

This PR also extends the support for the same to all of ScummVM's engines (stable & unstable). Work for ResidualVM's engines is in progress and a PR for the same will be opened soon. It should probably get merged around the same time when ResidualVM syncs up with ScummVM.

(*Some engines fallback detection is not linked statically. More info in a bit.)

New Structure

  • Addition of 2 new classes, MetaEngineConnect & AdvancedMetaEngineConnect.

    • MetaEngine: Houses all methods that are always linked statically. As before, each engine provides their own implementation of a MetaEngine, but only provides detection and related methods.

    • MetaEngineConnect: Houses all things that "can" be linked dynamically. These are bridge functions, that connect an Engine with MetaEngine, thus the name. It houses methods like createInstance, which instantiates an actual game engine.

    • AdvancedMetaEngine: Inherits from MetaEngine and provides a default detection method.

    • AdvancedMetaEngineConnect: Inherits MetaEngineConnect to derive a structure like before.

  • Most changes are centered around plugins. Two main new helpers are giveEngineFromMetaEngine and giveMetaEngineFromEngine which do the same thing as they sound like. These help out in various scenarios where you would need the matching engine-metaengine to do something.

The format of each individual engine is largely similar to before, but I will explain in a bit how some things will change.

Changes to existing engines

Most changes are pretty simple to follow. The new structure will need to be kept in mind when working on new engines,
To keep things short, I'll use "ME" for MetaEngine, "AME" for AdvancedMetaEngine, MEC & AMEC for the connect counterparts.
I'll also use the AGI engine as an example, so it's clear what everything means.

  • If an engine inherits ME, it should also inherit MEC.

  • The class ME for AGI lives in agi/detection.cpp, and inherits ME. The detection-related code goes here, detection tables, etc.
    Example:
    class AgiMetaEngine : public MetaEngine {...}

  • The same engine should use the equivalent meta-counterpart in agi/metaengine.cpp, a file specifically for MEC/AMEC.
    Example:
    class AgiMetaEngineConnect : public MetaEngineConnect {...}

  • If an engine uses AME, it should also use AMEC.


That's the basic engine split. As for the engines themselves,

  • Many engines have a custom description for the game. For example, in addition to the ADGameDescription struct, they would have their own, something like:
struct AGIGameDescription {
	ADGameDescription desc;

	int gameID;
	int gameType;
	uint32 features;
	uint16 version;
};
  • These go into agi/detection.h.

  • Sometimes the detection-tables will need some enumeration values present in the main engine header file, for example, agi.h. To avoid including the whole header which would contain irrelevant information for the executable, a copy of these enums is provided to both - the engine & metaengine.
    Simply shift these enumeration values to agi/detection.h. Include it in the main engine header, so everything returns to normal. However, now we can include it in the detection.cpp translation unit, while avoiding the engine header file.

  • Engines inheriting from MetaEngine will provide their own detection method. An example of this is Scumm. However, detection is not only used during detecting of games, but also during creating an instance. Scumm provides some internal static methods, called detectionImpl which is used both - by MetaEngine::detectGames(...) & by MetaEngineConnect::createInstance(...). The solution for this is to shift the common code inside another header file, detection_internal.h. Then, this is included by both - detection.cpp & metaengine.cpp

    • Remember to mark the methods as static inside the header, so the visibility is limited to the TU.
  • MetaEngines now go into the executable. They provide some naming functions - getName, getEngineId & getOrignalCopyright. Because MetaEngines will need to find a relevant MetaEngineConnect, the implemented MetaEngineConnect should override the getName method, and provide it a name similar to the one found in getEngineId of MetaEngine.

  • Lastly, after the creation of your files, you need to register them as plugins.

  • For detection.cpp, use REGISTER_PLUGIN_STATIC(...) macro, as they always go into the executable.

    • Keep in mind that the ID should be suffixed with "DETECTION". This is because the ID cannot conflict with the one in metaengine. The type should be a metaengine. Example:
REGISTER_PLUGIN_STATIC(AGI_DETECTION, PLUGIN_TYPE_METAENGINE, AgiMetaEngine);
  • For metaengine.cpp the story is as-is, if the plugin is dynamic, use the dynamic macro - otherwise the static version.

Module objects / Building detection features

  • Each engine's module.mk file will be adding object files to DETECTION_OBJS. It looks something like this:
...
...
# Include common rules
include $(srcdir)/rules.mk

# Detection objects
DETECT_OBJS += $(MODULE)/detection.o
  • Remember to add metaengine.o and remove detection.o from the enginename module.
  • Additional dependencies for detection are also ok. However, remember that engines could also use those files, so it wouldn't always be the case to separate the module-object completely. I will give some examples:
    • AGI uses something called wagparser to help in its fallback detection. The engine itself doesn't need it, so we can separate it completely i.e remove from MODULE_OBJS in engines/agi module and add it to DETECT_OBJS inside the same module list.
    • Scumm uses 2 files - file.cpp, file_nes.cpp to help with detection. However, the engines themselves need those files too, so we can't separate it completely. Hence, the updated module will look like
MODULE := engines/scumm

...
# Rest of the file
...

# Detection objects
DETECT_OBJS += $(MODULE)/detection.o

# Skip building the following objects if a static
# module is enabled, because it already has the contents.
ifneq ($(ENABLE_SCUMM), STATIC_PLUGIN)
DETECT_OBJS += $(MODULE)/file.o
DETECT_OBJS += $(MODULE)/file_nes.o
endif
  • The makefiles then take care of adding all the DETECT_OBJS from each of engine module files, regardless of which engine is being enabled or not.

Some engines that would require review

  • While most of the engines adapted pretty nicely to this structure, some had a somewhat different scenario because of which the base structure was changed a bit. I will list all of them below.

Sci & Wintermute (fallback-detection)

  • These engines have a struct similar to ADGameDescription, whose contents are overwritten by the fallback detection method. The problem is that these fallback detection methods are heavily dependent on engine resources.
    • I tried to make a "lite" version for these classes just to be helpful in detection, but that was a fast fail because it's too dependent on engine resources.
  • Thus, a workaround would need to be added.
  • A solution for this is that you would still override fallbackDetect in MetaEngine (which lives in detection.cpp, but instead of detecting games here, you provide a hook to the relevant engine plugin and call a relevant method from there.
    • Override a function called fallbackDetectExtern in the MetaEngineConnect class which lives in metaengine.cpp and provide the actual detection here, which is engine-resources dependant. See aryanrawlani28@eb960d0 for more info, and an in-code example.

Mohawk (dialogs)

  • A while back #2164 added support for in-game custom dialogs.
  • If you look at some of the engines currently scummvm master tree, which inherit from AME, you'll see that they provide some extra gui options via an array, ADExtraGuiOptionsMap gameGuiOptions[] and then pass this along to the AME constructor.
    • To not disturb the engine-structure too much, these are still as-is, meaning that without an engine plugin loaded, you'll be able to see these options. (Other things like Keymaps, achievements are still coming from the engine.)
  • Mohawk however, has the new dialogs in which they could query information from the game, then setup dialog accordingly. As such, it's probably best that they remain in the engine plugin itself.
  • So, ME's & MEC's both have the ability to create custom dialogs. ME's only provide the "Engine" tab inside "Edit Game" option, while MEC's provide in-game dialogs, etc.
  • For now, to clearly distinguish between them, the name buildEngineOptionsWidgetDynamic is given, but this is not really proper. extern & intern like the ones I did with fallback detection were ok, but it could be confusing in this scenario. Hence, this name can probably be changed into something better.

Detection features as a plugin themselves

  • On some low-hardware end platforms, the executable size bloat would prove difficult.
  • Making detection features available as a plugin itself would be beneficial in that scenario.

How is it done?

  • We already had the detection features separated from Engines. They are normally supposed to link directly with the executable, but since we have to have a plugin, we can group all the detection files themselves into a DETECTION type plugin.
  • In the ScummVM engines directory, look for detection.cpp. This is where we include the detection tables and mark the plugin as a PLUGIN_TYPE_DETECTION.
  • When using configure, --enable-detection-dynamic will be available, which enables the detection objects to be packed into a library.
    • When using this, helper methods in PluginManagerUncached will help in loading the detection library, linking the MetaEngines, and eventually clearing those out of memory once we don't need them.
    • If not using that, they will be linked statically, but still have an explicit option - --enable-detection-static available.

Modules objects / Building detection features for dynamic detection plugins

  • As said above, each engine is already populating the detect_objs variable.
  • In the configuration script, when enable-detection-dynamic is passed, the Makefiles.common includes the below module:
MODULE := detection // Only added if dynamic-detection enabled

DETECT_OBJS_DYNAMIC=$(addprefix ../,$(DETECT_OBJS)) // Since it lives outside the engines subdirectory, add "../" prefix.

MODULE_OBJS := \
	../engines/detection.o \
	$(DETECT_OBJS_DYNAMIC) // Reuse the already existing list of detection objects

# Reset detect objects, so none of them build into the executable.
DETECT_OBJS :=

PLUGIN := 1

# Include common rules
include $(srcdir)/rules.mk

As always, any suggestions, improvements, or anything really, is appreciated and helpful.

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Aug 12, 2020

DeepCode's analysis on #996dbd found:

  • ⚠️ 4 warnings, ℹ️ 1 minor issue. 👇
  • ✔️ 5 issues were fixed.

Top issues

Description Example fixes
Leaking memory. Glk::TADS::TADS3::TADS3 is allocated on the heap and never freed. In function createInstance. Occurrences: 🔧 Example fixes
Double delete. _thumbnail has already been deleted by . Occurrences: 🔧 Example fixes
Using sprintf can lead to severe buffer overflow vulnerabilities. Use the safe alternative snprintf instead. Occurrences: 🔧 Example fixes

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

@aryanrawlani28 aryanrawlani28 force-pushed the aryanrawlani28:move-detection-exe-v2 branch from 8d827fd to 1a8279c Aug 12, 2020
@henke37
Copy link
Contributor

@henke37 henke37 commented Aug 12, 2020

Not sure why you chose to use the word "give" instead of "get" as is custom in programming.

@aryanrawlani28
Copy link
Contributor Author

@aryanrawlani28 aryanrawlani28 commented Aug 12, 2020

Not sure why you chose to use the word "give" instead of "get" as is custom in programming.

There's already a "get" present. A "give" might be better for readability purposes and avoids ambiguity perhaps.

@henke37
Copy link
Contributor

@henke37 henke37 commented Aug 12, 2020

I think there is a greater risk of confusion if you mix verbs. And if there would be a conflict, it suggests that the feature exists already.

} while (!found && PluginMan.loadNextPlugin());

if (enginePlugin) {
warning("MetaEngine: %s \t matched to \t Engine: %s", plugin->getName(), enginePlugin->getFileName());

This comment has been minimized.

@mgerhardy

mgerhardy Aug 13, 2020
Contributor

Is that really a warning?

This comment has been minimized.

@aryanrawlani28

aryanrawlani28 Aug 13, 2020
Author Contributor

Ah, sorry, I guess I didn't point it out. Throughout the tree, I have placed many warning messages. These just exist for review/test purposes, because it will be way easier to track down what's happening if anyone faces any issues.

I plan to remove all unnecessary comments, warning statements once the PR looks nice and near merge with a single code cleanup commit. So, I'd request you to ignore these warning statements, they're just there temporarily (most of them anyway).

@sev-
Copy link
Member

@sev- sev- commented Aug 18, 2020

@aryanrawlani28 I am not sure who suggested you moving files around and creating detection/ directories here and there.

What is the purpose of this exercise?

@aryanrawlani28 aryanrawlani28 force-pushed the aryanrawlani28:move-detection-exe-v2 branch 7 times, most recently from bc66693 to 91ee6ff Aug 26, 2020
@aryanrawlani28 aryanrawlani28 force-pushed the aryanrawlani28:move-detection-exe-v2 branch 2 times, most recently from dc876a0 to 0886caa Sep 17, 2020
@sev-
Copy link
Member

@sev- sev- commented Sep 29, 2020

Hi Aryan,

I tested, it works nicely. Will start looking into the code deeper. These are the generic notes based on a cursory look and attentive reading of your comprehensive description above.

The PR got rot, again, sigh. I promise to merge it this week (if you have time to address my notes, of course).

First of all, I second @henke37 's opinion: the classic verb for is 'get', not 'give', thus, getEngineFromMetaEngine() and giveMetaEngineFromEngine() should be used.

Second, I was quite confused by naming of the MetaEngineConnect() and AdvancedMetaEngineConnect(). When reading your description and the code, it is not quite following their logic. E.g. you're asking to keep all MetaEngineConnect() methods in metaengine.cpp file. And MetaEngine() methods are kept normally in detection.cpp. My proposal here would be to call the detection part (MetaEngine now) as MetaEngineStatic or MetaEngineDetection. And the remaining dynamic methods will be in a class called plainly MetaEngine. Thus, current AgiMetaEngineConnect becomes AgiMetaEngine, and current AgiMetaEngine becomes AgiMetaEngineStatic. How does it sound?

Then, regarding the complex fallback detection in Wintermute and SCI engines now. I do not like the proposed solution of loading the plugin for detection, as this would clash with the purpose of this whole exercise. However, we do really need trying detecting it. Thus, I would do it this way: Have a setting in ConfMan.getBool("always_run_fallback_detection"), and set it to true on big platforms but on smaller platforms I would set it to false and do not attempt to load the plugin even if present. Also, the message could be improved like: "Engine plugin for Wintermute not present. Fallback detection is disabled.". The current message gives the impression that some known games are not detected.

@sev- sev- force-pushed the aryanrawlani28:move-detection-exe-v2 branch from cb3a0b9 to 22ab291 Sep 29, 2020
@sev-
Copy link
Member

@sev- sev- commented Sep 29, 2020

Putting detection into plugin does not compile.

I used

./configure --enable-detection-dynamic --enable-plugins --default-dynamic --disable-all-engines --enable-c++11 --enable-engine=director,scumm

Getting the following:

make: *** No rule to make target `detection/detection.o', needed by `plugins/detection.plugin'.  Stop.

This is after my attempt to fix it, as previously it was detection/../engines/detection.o

Copy link
Member

@sev- sev- left a comment

Very good quality. We are so close to the merge.

Please, do the class renames and address those few nit-picking comments and we merge it.

# Store original info
MODULES_ORIG:= $(MODULES)
MODULE_DIRS_ORIG := $(MODULE_DIRS)
KYRARPG_COMMON_OBJ_ORIG := $(KYRARPG_COMMON_OBJ)

This comment has been minimized.

@sev-

sev- Sep 29, 2020
Member

This looks like a leftover code.

This comment has been minimized.

@aryanrawlani28

aryanrawlani28 Oct 2, 2020
Author Contributor

Can you clarify what you mean?
By the time flow reaches here:

  • engines.mk file has been included and everything is going according to configuration scripts.
  • these lines stores the original information, preparing for all module.mk files to be included.
  • below, all module.mk files are included:
# Include all engine's module files, which populate DETECT_OBJS
-include $(srcdir)/engines/*/module.mk

which populate DETECT_OBJS, but they also fill other values like MODULES, MODULE_DIRS.

  • So, after my work of populating detect objs is done, I would need to reset values to the original configuration (how the conf script intended to do) like
# Reset stuff
MODULES := $(MODULES_ORIG)
Makefile Outdated Show resolved Hide resolved
MODULE_OBJS :=
MODULE_DIRS := $(MODULE_DIRS_ORIG)
PLUGIN :=
KYRARPG_COMMON_OBJ := $(KYRARPG_COMMON_OBJ_ORIG)

This comment has been minimized.

@sev-

sev- Sep 29, 2020
Member

This is the same leftover.

Makefile.common Outdated Show resolved Hide resolved
base/commandLine.cpp Outdated Show resolved Hide resolved
engines/dialogs.cpp Outdated Show resolved Hide resolved
engines/metaengine.cpp Outdated Show resolved Hide resolved
microtiles.o \
module.o \
module_scene.o \

This comment has been minimized.

@sev-

sev- Sep 29, 2020
Member

Why this rename?

This comment has been minimized.

@aryanrawlani28

aryanrawlani28 Oct 2, 2020
Author Contributor

My commit notes for this say:

  • The name was clashing with one of the audio files.
  • New changes were spread to the detection files, so no idea how this file is involved.
  • In the meanwhile, this commit should fix a build error, if any.

VStudio was having a problem with this file and one in the main scummvm subproject because they had the same name. I know we have the tool that checks for clashing names and renames it acc in dev tools, but this was still an issue and was the cause of compilation error.

gui/saveload-dialog.cpp Outdated Show resolved Hide resolved
@@ -77,14 +77,24 @@ Common::String SaveLoadChooser::createDefaultSaveDescription(const int slot) con

int SaveLoadChooser::runModalWithCurrentTarget() {
const Plugin *plugin = EngineMan.findPlugin(ConfMan.get("engineid"));

This comment has been minimized.

@sev-

sev- Sep 29, 2020
Member

You could avoid many renames if you would keep plugin variable and introduce metaEnginePlugin or something. This is for all these files in the GUI.

@sev-
Copy link
Member

@sev- sev- commented Sep 29, 2020

I could help with the renames if you like.

…es.h

- Currently, it is empty.
- After I enable engines one by to one to use detection statically, I will add them to the static_detect_engines array.
…n it.

Note: No detection objects added currently. It's just an empty variable uptill now.
- These DETECT_OBJS will be seen in action in the new commits
- They contain engine_name/detection.o
- They have MetaEngine code, which has detection features.
- This way, Executable will have linked against the detection.o files
- Detection.cpp files will be individually compilable and not dependent on engine
- MetaEngines will now always go to the executable.
- But, because they still live in engine projects and are connected via a macro, they will need to be differentiated from Engines.
- This macro, and it's use in future will help in that.
- Do NOT use pluginMan and load plugins, etc, etc.
- Get MetaEngines from memory, since they're always built in statically.
- Use detectGames method to detect games.
- The new implementation matchs a plugin from type MetaEngine to an Engine, and unloads every else ENGINE.
- Creating an instance of GLK requires searching through all sub-engine detection lists.
- So, these must be there at the time of creating an instance of GLK.
- Hence, all these objects will ONLY be skipped from static-detection, if GLK itself is static.
- The above point is because it will need to avoid duplicate symbols.
- Using static for plugins will result in crash if the plugins are unloaded after being called for the first time.
- Good example is when UncachedPluginMan uses these.
- The base class is temporarily named buildOptionsWidgetStatic.
- This allows apps that use create_project to build with statically linked detection features.
- Also add support to write an addtional file - detection_tables.h inside engines/.
- The name was clashing with one of the audio files.
- New changes were spread to the detection files, so no idea how this file is involved.
- In the meanwhile, this commit should fix a build error, if any.
- This should fix the build errors.
- No C11 support yet, revert to use map to fix build
@aryanrawlani28 aryanrawlani28 force-pushed the aryanrawlani28:move-detection-exe-v2 branch from 22ab291 to 978c2b8 Sep 30, 2020
- ME -> MetaEngineStatic (static parts)
- MEC -> MetaEngine (dynamic parts)
- ME -> MetaEngineStatic (static parts)
- MEC -> MetaEngine (dynamic parts)
- getME -> getMetaEngineStatic (static parts)
- getMEC -> getMetaEngine (dynamic parts)
- DetectionConnect -> DetectionDynamic
@aryanrawlani28 aryanrawlani28 force-pushed the aryanrawlani28:move-detection-exe-v2 branch from 7383474 to 996dbdf Oct 2, 2020
@sev- sev- merged commit a8a4c25 into scummvm:master Oct 3, 2020
4 of 5 checks passed
4 of 5 checks passed
Windows (win32, x86-windows, x86, --enable-faad --enable-mpeg2 --enable-discord --disable-fribidi...
Details
Windows (x64, x64, x64-windows, --enable-faad --enable-mpeg2 --enable-discord --disable-fribidi, ...
Details
Windows (arm64, arm64, arm64-windows, --enable-faad --enable-mpeg2 --enable-discord --disable-fri...
Details
deepcode-ci-bot Found 4 warning issues, 1 minor issue.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
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

4 participants
You can’t perform that action at this time.