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

Replace priorities with groups #20

Closed
Ortham opened this issue Feb 18, 2018 · 17 comments
Closed

Replace priorities with groups #20

Ortham opened this issue Feb 18, 2018 · 17 comments
Assignees

Comments

@Ortham
Copy link
Member

Ortham commented Feb 18, 2018

Priorities in LOOT's metadata have always been a pain point, as:

  • they're a bit abstract
  • local vs. global can be confusing
  • inheritance of priority values is difficult to reason about

Priorities were introduced to solve the problem of "how do I say plugin X needs to load late, after most but perhaps not all plugins, without having an enormous after list?" and similar for loading plugins early (which is actually the messier problem, as there's no before). In BOSS this was manually solved by dividing the masterlist into themed sections, and while this wasn't a suitable solution for LOOT, perhaps it's worth taking some inspiration from it.

It may be possible to replace priorities with groups, which would act a bit like global priorities but be more accessible. For example:

groups:
  - name: default
    after:
      - Unofficial patches
  - name: &mapMarkersGroup Map Markers # Create an anchor to the group name
    after:
      - default
  - name: Unofficial patches Has an implicit after: []

plugins:
  - name: Unofficial Patch.esp
    group: Unofficial patches
  - name: foo.esp # Belongs to the 'default' group
  - name: map marker.esp
    group: Map Markers
  - name: other map marker.esp
    group: *mapMarkersGroup # To prevent typos, references can be used

this is equivalent to:

plugins:
  - name: Unofficial Patch.esp
  - name: foo.esp
    after:
      - Unofficial Patch.esp
  - name: map marker.esp
    after:
      - Unofficial Patch.esp
      - foo.esp
  - name: other map marker.esp
    after:
      - Unofficial Patch.esp
      - foo.esp

and is similar to but not the same as:

plugins:
  - name: Unofficial Patch.esp
    global_priority: -1
  - name: foo.esp # global_priority is 0
  - name: map marker.esp
    global_priority: 1
  - name: other map marker.esp
    global_priority: 1

Defining the above:

  • Groups are identified by name strings.
  • Groups can load after other groups.
  • Groups load after nothing by default, i.e. [] is the default value for the after key.
  • Groups don't need to be defined before they're referenced.
  • A plugin can belong to a single group.
  • All plugins would belong to the default group by default.
  • The default group doesn't need to be declared in groups.
  • The default group can be declared in groups with a non-empty after array.
  • A plugin group value that doesn't match a group named in groups would cause a sorting error.
  • A group in a group's after list that doesn't match a named group would cause a sorting error.
  • During sorting, groups would recursively resolve to sets of plugins, and a plugin would add all the plugins sets its group loads after to its after metadata. Resolution needs to be recursive as without it if (using the above example) foo.esp is not installed, map marker.esp wouldn't know to load after Unofficial Patch.esp.
  • Userlists would be able to change a plugin's group, add new groups and extend existing groups' after lists. In keeping with userlist behaviour for other metadata, it would not be possible to remove entries from a group's after list.

Behavioural differences vs. priorities:

  • Groups ignore whether or not plugins conflict, i.e. they're more like global priorities than local priorities.
  • Priorities can't cause cycle errors as they are only applied if they don't contradict a plugin's masters, req or after metadata, but groups resolve to after metadata so can cause cycles. Using the above example, if foo.esp had an after for map marker.esp, that would cause an error.

A few other points worth noting:

  • As group identifiers are strings, there's no need to think about making good use of the available range of values, it's always trivial to insert groups.
  • Because all the groups (apart from default) are named and declared in the same place (in a given metadata file), it's easy to see what groups are available and get an idea of what they're for.
@Ortham Ortham self-assigned this Feb 18, 2018
@Ortham
Copy link
Member Author

Ortham commented Feb 18, 2018

@TanninOne you've had problems with priorities recently, what's your opinion of this idea?

@Ortham
Copy link
Member Author

Ortham commented Feb 18, 2018

I've changed referencing an undefined group to be a sorting error rather than a parsing error so that userlists can extend/override the defined groups and masterlist groups can be referenced in the userlist.

@TanninOne
Copy link

I think this should be a lot more intuitive to users.
For Vortex I've actually thought of mapping certain pre-defined priorities to names according to this documentation page: https://loot.github.io/docs/contributing/Global-Priorities.html

The biggest problem I see is that right now very few plugins have any priority or rules assigned. Once this feature gets added users will probably expect every plugin to be categorised or they will do it themselves. This may lead to increased maintenance work on the masterlist.

The next thing that may be a problem is that the rule on a plugin disagrees with the rules between groups. That would cause a plugin to no longer appear inside its group. Then the user's OCD kicks in and they start fight the sorting to "repair" the visual order.
I'm somewhat conflicted on this. On one hand I'm a firm believer that visual ordering and load ordering with the goal of keeping the game stable should be strictly separated, on the other hand there is a causal connection between plugin group and their dependencies.

@Ortham
Copy link
Member Author

Ortham commented Feb 19, 2018

The biggest problem I see is that right now very few plugins have any priority or rules assigned. Once this feature gets added users will probably expect every plugin to be categorised or they will do it themselves. This may lead to increased maintenance work on the masterlist.

I think this is an education problem, similar to how plugin masters don't need to be added to req. If a user wants to create groups for everything in their load order even if it's pointless, that's fine, but upstreaming that into the masterlist wouldn't add any more maintenance work than challenging the need and rejecting PRs that couldn't justify it: I'm not too concerned about increased masterlist activity as a result of this.

The next thing that may be a problem is that the rule on a plugin disagrees with the rules between groups. That would cause a plugin to no longer appear inside its group. Then the user's OCD kicks in and they start fight the sorting to "repair" the visual order.

I may have misunderstood what you mean, but I don't think it would be possible for a plugin to not appear inside its group. This change would mean that all load-order-affecting metadata sits on equal footing (because groups resolve to after metadata), so that contradictory metadata is always a hard error. I've reworded the OP and added another example to help clarify this.

@TanninOne
Copy link

Ok, if you're not worried about masterlist activity. :)

Regarding the second point, I guess it was me who understood you.
So if we have

groups:
  - name: First Group
  - name: Second Group
    after:
      - First Group

plugins:
  - name: foo.esp
    group: First Group
    after:
      - bar.esp
  - name: bar.esp
    group: Second Group

That would cause a cyclic dependency error during sorting because bar.esp has an inherited "after everything in First Group".
Yeah, then what I thought of isn't a problem.
It does then mean however that for foo.esp to load after bar.esp, our user has to
create a new group (since the default group would load first), assign foo.esp to that and then add a rule to load the new group after Second Group.
If this was the only adjustment you wanted to make you end up with a group that consists of only one Plugin.

Also: let's assume we starting from the existing master- and userlists. You create on group and assign any plugin (say foo.esp) to it. Now every rule "after: - foo.esp" becomes a cyclic dependency because all other mods still belong to the default group. So now you have to put every plugin with such a rule into their own group so they can load after foo.esp which means all rules referring to them are cyclic.

I don't have a great overview of the masterlists, maybe this would just be a minor overhaul.

Hrm, still: It should be easy enough to visualise and assign groups quickly via drag&drop.

@Ortham
Copy link
Member Author

Ortham commented Feb 19, 2018

That would cause a cyclic dependency error during sorting because bar.esp has an inherited "after everything in First Group".

Yes, that's correct.

If this was the only adjustment you wanted to make you end up with a group that consists of only one Plugin.

In general, I don't expect creating new small groups to have much overhead, I suspect the user is more likely to run into trouble reasoning about the increased (and potentially unnecessary) level of abstraction first.

You create on group and assign any plugin (say foo.esp) to it. Now every rule "after: - foo.esp" becomes a cyclic dependency because all other mods still belong to the default group.

No, because groups don't load after default by default.

groups:
  - name: foo_group

plugins:
  - name: foo.esp
    group: foo_group
  - name: bar.esp
    after: [ foo.esp ]

is equivalent to

plugins:
  - name: foo.esp
  - name: bar.esp
    after: [ foo.esp ]

because two unrelated groups might as well be the same group.

@Ortham
Copy link
Member Author

Ortham commented Feb 19, 2018

I've thought through a few user stories, I'm not sure if I've missed any.

User wants to load a plugin as part of an existing group

plugins:
  - name: plugin.esp
    group: default # Could be any group in the masterlist

User wants to load a plugin after an existing group, but no group doing so exists

groups:
  - name: new group
    after: [ default ] # Could be any group in the masterlist

plugins:
  - name: plugin.esp
    group: new group

User wants to load a plugin before an existing group, but no group doing so exists

groups:
  - name: new group
  - name: default
    after: [ new group ] # This should extend not override the masterlist definition to avoid breakage

plugins:
  - name: plugin.esp
    group: new group

The default group definition would need to extend, not override, the masterlist definition, as otherwise an incomplete definition (and definitions could become incomplete with future changes to the masterlist) could lead to breakage.

User wants an existing group to not load before/after a group it currently loads before/after

Can't be done - submit a PR to the masterlist or create a new group and put the
relevant plugins in it instead.

@TanninOne
Copy link

Ok, sorry, I took

All plugins would belong to the default group by default.
The default group doesn't need to be declared in groups, but can be if it should load after other groups.

to mean that each declared group implicitly loads after default unless something else was specified.
So if I understood this correctly, plugins in the default group practically "flow" around plugins in unrelated groups but one can still set up a group to load after the default group like every to make it load last.
Sounds good to me.

User wants an existing group to not load before/after a group it current loads before/after
Can't be done - submit a PR to the masterlist or create a new group and put the
relevant plugins in it instead.

Somewhat unrelated but is such a feature something that could be added or is it omitted intentionally? Something like a "not_after" one can put in the userlist to disable a rule from the masterlist?

@Ortham
Copy link
Member Author

Ortham commented Feb 19, 2018

Somewhat unrelated but is such a feature something that could be added or is it omitted intentionally? Something like a "not_after" one can put in the userlist to disable a rule from the masterlist?

I'm not against having separate syntax to explicitly override/opt-out of masterlist metadata, it's more about preventing the user from accidentally doing so using the same syntax as adding new metadata. Though in general I'd prefer the masterlist updated so that overriding/opting out is unnecessary, as it indicates that the masterlist metadata is incorrect or overly restrictive.

@TanninOne
Copy link

I don't disagree: If someone finds the masterlist to be wrong they should be able to explain why/how and then get the masterlist fixed.
But in practice a user may just be trying out things without actually being sure the rule is wrong.
Plus many users just like the knowledge they can control everything, even if they don't need it.
So personally I'd love if there was a way to disable certain rules (explicitly) but I fully understand if you say that's not how loot is intended to be used.

@Ortham
Copy link
Member Author

Ortham commented Feb 19, 2018

Hmm, I see what you mean. I'll think about it, but it's an issue that touches more than just priorities, so I'd prefer to consider it independently, and won't include such functionality as part of this.

@Ortham
Copy link
Member Author

Ortham commented Mar 18, 2018

I've just noticed a significant flaw in this feature as currently described: because everything is in the default group by default, adding a non-master to a group that loads before default will cause a cyclic error if default contains a master.

There's a similar problem with plugins' masters and req and after metadata. These are more complex to solve, because they can form dependency trees. For example, given A.esp, B.esp, ..., Z.esp, where each plugin has the preceding plugin as its only master, Z.esp could be in an early group that default loads after and A.esp could be implicitly in default. This would cause a cycle, and there's no way to know just looking at A.esp and Z.esp, only searching the tree between them would find it.

The underlying issue is that implicitly putting everything in the default group means default metadata can result in a cycle, which is at best unexpected. The best solution I can think of for this is to ignore group metadata when belonging to the default group would cause a cycle. This is hopefully intuitive enough that the added complexity won't significantly complicate reasoning about groups.

This would mean that before.esp and default.esm (belonging to groups of the same names) would not cause a cycle, because LOOT would see default.esm belongs to the default group. On the other hand, before.esp and after.esm would cause a cycle, and one would have to be moved to a different group to resolve it.

In the A...Z example above, there wouldn't be a cycle as A.esp's membership of default would be ignored, but only for the purposes of the relationship between it and Z.esp. If there was an unrelated B.esp that the default group loads before/after, A.esp would still load before/after B.esp.

Although in simple cases (like master flag differences) this solution should be fairly intuitive, I expect it'll cause some non-obvious behaviour in more complex cases, and default being a special case could cause confusion. Alternatives I came up with are:

  • A more permissive version that ignores all group metadata that causes cycles, not just the default group. This is probably more likely to introduce unexpected behaviour, but would be less likely to break things when converting from priorities to groups.
  • A plugin could inherit the group of whatever plugins it's supposed to load after. This is what priorities currently do, it's a lot more complex, without any clear advantages.

@TanninOne
Copy link

Imho the second solution of inheriting groups sounds very unintuitive.
I feel it would be understandable that a rule that targets a specific plugin would take precedence over one that affects whole groups.

What about introducing a general concept of "soft" rules which will only be applied if they don't contradict/cause a cycle with a "hard" rule.
Group rules could be, by default, soft rules, plugin specific rules would default to hard rule but both could be toggled by a flag.
That way we could enforce group rules for specific groups but that would be a explicit choice.
We could also (probably mostly in user lists) set soft rules for individual plugins if the rule is only "cosmetic".

@Ortham
Copy link
Member Author

Ortham commented Mar 19, 2018

I'm now leaning towards not having the default group be treated specially, and just ignore all group-based relations when they'd cause a cycle, because that's more consistent and so less likely to trip people up.

What about introducing a general concept of "soft" rules which will only be applied if they don't contradict/cause a cycle with a "hard" rule.

I don't think I want to take the complexity hit of allowing an arbitrary rule to be hard or soft (and it's certainly a bigger idea than fits in this issue), but that's a good way to think about / explain groups.

However, groups being soft seems like a workaround for a system that is self-contradictory, and I'd rather solve that contradiction if possible (it's probably not), as soft groups could have knock-on effects I haven't thought of.

One thing that's currently bothering me is that if groups can be ignored if they cause cycles, then the order of evaluation can affect the result of sorting. For example, given plugins A.esp, B.esp and C.esp in groups of the same names, if:

  • group B loads after group A
  • group C loads after group B
  • C.esp is a master of A.esp

The load order could be C.esp -> A.esp -> B.esp or B.esp -> C.esp -> A.esp depending on which group rule was evaluated first. I don't really like this, but haven't yet thought how it should be handled.

Evaluation order is stable and already matters for adding overlap and tie-break edges, so it's not the end of the world, but having metadata ignored just because LOOT happened to get to it after some other metadata seems more of a problem.

@Ortham
Copy link
Member Author

Ortham commented Mar 19, 2018

The issue detailed in the previous comment could be avoided if group cycle handling is adjusted so that a group-derived rule is ignored if it would introduce a cycle in the absence of other group-derived rules. The goal is to ensure that the evaluation order of group-derived rules doesn't matter, just as it doesn't matter for plugin-data-derived and other metadata-derived rules.

With this adjustment, the example given in the previous comment would cause a sorting error, because the group-derived rules are both applied. From the user's perspective this isn't great, but I think it's better than LOOT making an unreasoned choice over which to apply.

To put it another way, LOOT currently requires that the set of (masters, master flags, after and req)-derived rules is acyclic, and that groups are acyclic, this would add the requirement that group-derived rules are non-conflicting when applied on top of the "hard" rules.

If I've thought through this correctly, it avoids all the previously-raised issues, and seems compatible with the simple priority -> group conversions I've done in the masterlists so far.

This could be implemented by applying group-derived rules in two passes: in the first pass, each rule is checked to see if adding it would cause a cycle, and if not it's recorded. The second pass would then apply the recorded rules, and then the cycle checker would run as it currently does.

@Ortham
Copy link
Member Author

Ortham commented Mar 21, 2018

A couple more things:

  • missing group errors need to be propagated better, at the moment the exception just carries a message and as this isn't localisable, LOOT won't display it in the UI, so the user doesn't know why something has gone wrong without opening the log.
  • I'm not entirely sure about how to map local priorities to groups. Initially I just tried a straight replace with a group for the equivalent global priority, but I don't think that's right, so I reverted that and now the masterlist v0.13 branches only have global priorities replaced. It needs more thought, and probably a review of how the priorities are being used across the different masterlists.

@Ortham
Copy link
Member Author

Ortham commented Mar 28, 2018

Closing this as I've merged groups and removed priorities as of 523dc87.

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

No branches or pull requests

2 participants