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

Fix mod selection not working with base types #9571

Merged
merged 2 commits into from
Jul 16, 2020

Conversation

smoogipoo
Copy link
Contributor

Fixes the issue mentioned in #9538

I ported back the mod inheritance from the PR above. This is a case where we're trying to select HD, but since FI : HD and FI is the first visible button, FI is re-selected. I'd say it makes sense to do exact matching for mod selection, and keep InstanceOfType for deselection only.

@bdach
Copy link
Collaborator

bdach commented Jul 15, 2020

I'd say it makes sense to do exact matching for mod selection, and keep InstanceOfType for deselection only.

Not sure I'd agree with that notion; I'd say it needs to either always respect type hierarchy or always match by outermost type. That said I see that changing to that model utterly breaks mod incompatibility logic right now, but I would argue that's a shortcoming of the incompatibility rules' design in general and should be properly addressed with #7155, so not sure if this is something we want to deal with right now.

@smoogipoo
Copy link
Contributor Author

smoogipoo commented Jul 16, 2020

The way I see it is deselection rules are user defined - a mod can be incompatible with a specific mod or with an entire subset of mods without ambiguity, whereas selection can't be defined in the same way and introduces ambiguity when you request selection on a subset.

tl;dr the only way selection can work is by exact matching. As for deselection, is there a reason why you'd limit it to exact matching? There are times when it may be useful - if a ruleset made a mod incompatible with ModRateAdjust and listed DT,HD, would you expect the ruleset to then have to explicitly add more exclusions if we added another game-wide ModRateAdjust mod?

@peppy
Copy link
Member

peppy commented Jul 16, 2020

I think this is fine for now, until a case comes up where we need otherwise (I can't think of one).

@peppy peppy merged commit 61b298d into ppy:master Jul 16, 2020
@peppy peppy deleted the fix-mod-selection branch July 16, 2020 07:04
@bdach
Copy link
Collaborator

bdach commented Jul 16, 2020

The reason I'd want to see deselection on concrete types too is mostly consistency/principle of least astonishment. I do realise that inheritance is often leveraged to avoid having to specify subtypes in incompatibility rules, but again, I'd mostly say it's a shortcoming of how these rules are implemented right now.

Having one central place that declares them, perhaps in a more expressive (and less repetitive) manner than done currently, could deal with that, but I'd have to take one long hard look at what mods we have right now to say for sure.

@peppy
Copy link
Member

peppy commented Jul 16, 2020

The whole mod manager class needs to be split out as it is managing drawable and non-drawable aspects, so we can definitely revisit and better define the API at that point.

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.

3 participants