flareteam / flare-engine

Free/Libre Action Roleplaying Engine (engine only)
http://flarerpg.org/
GNU General Public License v3.0
1.11k stars 187 forks source link

Trying to understand the handling of multiple mods with the same name #1840

Closed Daggersoath closed 1 year ago

Daggersoath commented 1 year ago

I am currently working on a heavily modified version of the engine, however during some modifications to the ModManager I have noticed there is no real handling of multiple mods with the same name, and so was curious if anyone knew what the intended behaviour would be? So the flow as I can see it currently, we go through the 2 mod directories and gather the names of all mods and remove duplicates. eg. path_data:

path_user:

This will get converted to a list of [mod_a, mod_b, mod_c], which is fine initially, but then when it comes to using the mod, we use a for loop on mod_paths and act on every instance of the mod.

This happens in 3 cases, loadMod, locate and list.

I am mainly looking for what the intended interaction here is so I can follow that in my own development, but potentially could be something that was just overlooked and should have an error during startup?

dorkster commented 1 year ago

In the case of "loadMod" it tries to parse the settings file for each of the instances of the mod. Forseeably this could cause a couple of issues, if the two mods require different versions of things or have different dependencies, it isn't sure that everything will be loaded properly.

I don't think there's any reason that the settings.txt file for all instances of a given mod are parsed here. So this could just be a bug. It should be more like locate(), which only grabs the file from the highest priority mod that it can.

In the case of "locate", we find the first instance of the file we are looking for and return that, which could be fine, but conflicts with both loadMod and "list", which risks mixing files from the 2 mods.

This is generally speaking the desired behavior for loading mod files. The order goes:

  1. custom_path_data (--data-path command line flag)
  2. path_user
  3. path_data (if custom_path_data isn't set)

In the case of "list" we add all the paths for mods with the specific name and then remove duplicates. Which could easily add files from both mods if some files only existed in the first and not the second.

list() behaves just like locate(), with one exception. When full_paths is true, we do indeed include the paths from each instance of a given mod. The primary use of this functionality is the FileParser's "APPEND" feature. Let me give an example:

path_data/mods/mod_a/my_file.txt

key1=foo
key2=bar

path_user/mods/mod_a/my_file.txt

APPEND
key3=10

When the engine goes to parse my_file.txt, it will look like this:

key1=foo
key2=bar
key3=10

This allows modders to extend a text file without having to outright duplicate it. For example, a mod that adds additional items to items/items.txt. The only way this can be accomplished is by parsing every instance of the file.

Daggersoath commented 1 year ago

I don't think there's any reason that the settings.txt file for all instances of a given mod are parsed here. So this could just be a bug. It should be more like locate(), which only grabs the file from the highest priority mod that it can.

If as you mentioned there are multiple of other files that are used to append, could it perhaps be needed to load each to update the required versions of things? (eg in the case of multiple items files, the append version of the file needs an updated version of another mod).

Currently it runs from lowest priority to highest and updates the properties with each settings file it finds (ModManager.cpp:334).

Also thanks for helping me clarify things! :)

dorkster commented 1 year ago

If as you mentioned there are multiple of other files that are used to append, could it perhaps be needed to load each to update the required versions of things? (eg in the case of multiple items files, the append version of the file needs an updated version of another mod).

The problem here is that the settings.txt file is always "appended" to the lower priority mod of the same name. This is unintended.

Daggersoath commented 1 year ago

The problem here is that the settings.txt file is always "appended" to the lower priority mod of the same name. This is unintended.

Ah, so I guess then it should search for the file in priority order and break after it finds the first one? Would this also apply to the gameplay.txt file? I'm assuming it unlikely that an append mod is unlikely to change the enable_gameplay state of a mod but you never know.

dorkster commented 1 year ago

Would this also apply to the gameplay.txt file? I'm assuming it unlikely that an append mod is unlikely to change the enable_gameplay state of a mod but you never know.

Practically, there's no difference between appending and overwriting engine/gameplay.txt, since it only contains a single property. So yeah, I would say it should behave the same there.