Improve incompatible mods logic

[peppy]

• There should be a central method for checking and removing incompatible mods.
• A mod should be able to list itself as incompatible but still be selectable. Consider a case where you have abstract ModTimeAdjust and want to make it incompatible with all other ModTimeAdjust inheritors. Or maybe even IApplicableToX.
• Look into whether we can list incompatible pairs, rather than requiring it to be specified in two different places (prone to error).

[huoyaoyuan]

I thought about this several days ago.

abstract class Mod
{
    abstract bool IsCompatibleWith(Mod other);
}

abstract class ModTimeAdjust
{
    override bool IsCompatibleWith(Mod other) => !(other is ModTimeAdjust);
}

[MightyHelper]

Maybe create "incompatibility groups"?
They would consist of an array of mods that are incompatible between each other, in other words, only one of the group can be active at any point in time.
Haven't read the code, just an idea I came up with after reading the issue.

[Flutterish]

In general mods are definitely incompatible if they modify the same values plus some other cases. You could somehow use reflections to mark certain fields / properties as mod modifiable, then mods that modify the same values are incompatible.

// some file
[Moddable( nameof( SpeedModifier ) )]
private double SpeedModifier = 1;

// mod file
[Modifies( /*nameof( MyOtherFile ),*/ nameof( MyOtherFile.SpeedModifier ) )]
private double SpeedModifier => 2;

Do you think this idea is viable?

[bdach]

That just sounds like inviting unnecessary complexity to me.

[smoogipoo]

What about for cases that mods aren't inter-compatible between themselves without touching an external property?

I think as mentioned in the OP we just need somewhere to define incompatibility pairs - probably a method from Ruleset.

[Flutterish]

Then you need an additional method which explicity states that, like the current one or huoyaoyuan's does.
As of bdach's comment, I think this is pretty similar to [Cached] and [Resolved] in the sense that it solves a dependency in the background.

Id like to add that my suggestion is not a whole solution, only a part of it.

[smoogipoo]

There isn't a direct mod-to-property relationship. Mods are extensible to the point they can do literally anything anywhere from the beatmap to the draw hierarchy without setting flags in random places. An automated system just does not have enough knowledge to determine incompatibilities with such a system - it requires developer knowledge.

[Flutterish]

As I said, this is not a full solution, it's more of a convinience tool like dependency loading. It allows you to know that 2 mods can't work together if they use colliding properties, but there still needs to be a way to note more complex incompatibilities.

[bdach]

I would propose that you try writing such a system yourself and seeing how it works in practice. I strongly believe that you'll find a solution like that would be miles more complicated and unwieldy than just adding one central place that allows for specifying incompatibility pairs/groups.

Backseating like this isn't very helpful. To quote Linus Torvalds, "Talk is cheap. Show me the code."

[Flutterish]

Have a look at Hitokori/AutoMods at Mods/AutoImplementedMod and Mods/ModCompatibility

[bdach]

How is that supposed to be a complete proposal?

As far as I can see there's no actual usage of ModifiesAttribute (the decision to use a plain string for Name is incredibly suspect for me too, but that's another thing). Therefore the proposal can't even be properly discussed since you haven't actually tried to replace the existing incompatibility system with your proposed one in a meaningful way.

Let me rephrase: if you want this to happen your way, please submit a proposal as a PR that can be upstreamed, and we'll discuss its merits and drawbacks properly once it's actually something that works and replaces what we have right now. Until then I don't think any of us are interested in discussing this further.