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

Use virtual paths to specify exact mod to enable #11784

Merged
merged 7 commits into from Jan 30, 2022

Conversation

rubenwardy
Copy link
Member

@rubenwardy rubenwardy commented Nov 21, 2021

Currently, world.mt uses true or false to enable a mod based on its name. When there are multiple mods with the same name, Minetest will fail to load. This isn't very user friendly or obvious at all

This PR uses virtual paths like mods/modpack1/moddir to enable the mod instead of true. This allows exact copies of the mod to be enabled

load_mod_mymod = mods/modpack1/moddir

Thanks to pgimeno for paving the way for this

Fixes #4183

To do

  • Implement UI
  • share/ and env/ support
  • Migrate mod if no paths found

How to test

Here are 3 mods in 2 modpacks, all with the same name

load_mod_test.zip

@rubenwardy rubenwardy added @ Startup / Config / Util WIP The PR is still being worked on by its author and not ready yet. Bugfix 🐛 PRs that fix a bug Feature ✨ PRs that add or enhance a feature @ Mainmenu @ Content / PkgMgr and removed WIP The PR is still being worked on by its author and not ready yet. labels Nov 21, 2021
@rubenwardy rubenwardy marked this pull request as ready for review November 21, 2021 16:03
src/content/mods.cpp Outdated Show resolved Hide resolved
@rubenwardy
Copy link
Member Author

rubenwardy commented Nov 24, 2021

One potential UX shortfall here: if the virtual path doesn't exist, the mod just won't load. There will be an errorstream log that lists possible other paths for the mod, but the game itself will still load. This would make naively sharing worlds hard, as different players may have mods installed to different locations

Solutions:

  1. Do nothing / fix this later
  2. Throw an error rather than continuing to load the game, let the player sort it out (to disable deleted mods, you would now need to open Config Mods and then click Save, which isn't very intuitive)
  3. Automatically select other mods with the same name when loading the game (errr.... potentially dodgy)
  4. Automatically select other mods with the same name when opening config mods (probably better, if there's a warning)

My preference would be 1 or 4

@rubenwardy rubenwardy added the WIP The PR is still being worked on by its author and not ready yet. label Nov 26, 2021
@rubenwardy rubenwardy added Testing needed and removed WIP The PR is still being worked on by its author and not ready yet. labels Dec 7, 2021
@SmallJoker
Copy link
Member

SmallJoker commented Dec 7, 2021

How about "4", but only do the automatic selection if there's exactly one mod with the matching name? Otherwise show a warning.

@rubenwardy
Copy link
Member Author

How about "4", but only do the automatic selection if there's exactly one mod with the matching name? Otherwise show a warning.

Done - it now automatically selects a mod in a different path if there's only one mod with that name present. I haven't handled the case with multiple mods because I'd like to improve the usability of the config mods dialog in the future, and don't want to make this PR much more complicated

@SmallJoker
Copy link
Member

SmallJoker commented Jan 2, 2022

Breaks CSMs:

...
2022-01-02 20:58:43: WARNING[Main]: Mod name conflict detected: "colored_names"
2022-01-02 20:58:43: WARNING[Main]: Will not load: /data/Minetest/run/bin/../clientmods/colored_names
2022-01-02 20:58:43: WARNING[Main]: Will not load: /data/Minetest/run/bin/../clientmods/colored_names
2022-01-02 20:58:43: WARNING[Main]: Mod name conflict detected: "inspector"
2022-01-02 20:58:43: WARNING[Main]: Will not load: /data/Minetest/run/bin/../clientmods/inspector
2022-01-02 20:58:43: WARNING[Main]: Will not load: /data/Minetest/run/bin/../clientmods/inspector
...
2022-01-02 20:58:43: ERROR[Main]: ModError: Unresolved name conflicts for mods XXXXXXXXXX, "inspector", "colored_names", XXXXXXX

RUN_IN_PLACE=1

@rubenwardy
Copy link
Member Author

I blissfully forgot that CSMs were a thing

@rubenwardy rubenwardy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 2, 2022
@rubenwardy
Copy link
Member Author

rubenwardy commented Jan 6, 2022

Breaks CSMs:
....

Fixed. It's because share defaults to user when RUN_IN_PLACE=1, which results in a duplicated search path.

I'm currently using a map from virtual_path to actual_path, which means there's no unique constraint on actual_path. Really, both need unique constraints. Maybe a set of pairs or something would be more suited

@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 6, 2022
@SmallJoker
Copy link
Member

grafik
The main menu is broken. It lists all mods as enabled. Workaround: enable all, disable all. The PR seems to work well other than in this aspect.

@rubenwardy
Copy link
Member Author

The main menu is broken. It lists all mods as enabled. Workaround: enable all, disable all. The PR seems to work well other than in this aspect.

Fixed, was missing a check on the value in world.mt

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works-

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mod conflicts if same mod in many modpacks
4 participants