Electrocutor commented 6 years ago

Have NEEDS be the only dependency checker.

Have BEFORE and AFTER allow comma-separated lists of optional mods/patches. If they exist, it will use them for ordering, if not it will ignore them.

blowfishpro commented 6 years ago

Would you mind walking us through the use case you have in mind?

Electrocutor commented 6 years ago

1 it would prevent the chaos that is every modder trying to come up with the correct name for FOR to make sure their patch ends up in the right place.

An actual use-case would be on my color tinting patch. If B9Switch or Firespitter exist, then the patch must run after them because the ModulePartVariant must be added/happen after they have finished their PartModule changes.

Another use case would be anything that needs to wait for all of whatever potential patches to be made before proceeding, such as with Vens model changes. If your patch is reliant on the model pathing, then it must happen after Vens, but not require that Vens had changed anything. The exact case would be the setting of TexturesUnlimited metallic and smoothness properties; I use a search against any Parts that contain a model that resides in the /Squad/ path.

Basically, the patch syntax requires that it happen after whatever potential changes have already been done by patches present in mods A, B, and C; but if none of those are present, it happens against the stock values.

Electrocutor commented 6 years ago

Example of FOR chaos: Vens part data is "VenStockRevamp" Vens model path patch is FOR "zzzVSRPathPatch"

So I had to add a FOR to my TU patch named "zzzzTUStock" to make sure it happened after "zzzVSRPathPatch", but not requiring that Vens was present.

Should someone make a variant patch for Vens, then it would likely be named something like FOR "zzzzVSRVariants", in which case I would have to rename my patch as FOR "zzzzzTUStock".

You can see how it would get out of hand.

If BEFORE and AFTER did not have dependency built-in, then I could simply have: AFTER[zzzVSRPathPatch]

then, if someone made a variant patch, I could change it to: AFTER[VSRVariants] (because VSRVariants would already have an AFTER[zzzVSRPathPatch] within it)

What's more, having removed the FOR issue, I could simply name my patch "TU_Stock".

blowfishpro commented 6 years ago

Having patches apply in a non-deterministic way worries me. It's already hard enough to debug when patches don't work because of patch order - if the order is now dependent on what mods you have installed it will get even worse. The zzzzz* arms race is annoying too, but at least it's relatively easy to debug.
Making AFTER/BEFORE not check for dependencies would constitute a breaking change. A few months ago I introduced a breaking change in MM and that got a lot of push back from the modding community - and that was for behavior that wasn't supported to begin with. You're talking about making a breaking change to behavior that is currently supported.
Once #96 is implemented, it will hopefully reduce the amount of zzzzz-ing that has to happen.

theonegalen commented 6 years ago

Yeah, removing dependency checking from BEFORE and AFTER would be a freaking nightmare for all tech tree mods, and for me especially.

Electrocutor commented 6 years ago

"Having patches apply in a non-deterministic way worries me." This would not be the case. They would be alphabetical if no BEFORE/AFTER were specified and they would always be after any identifiers specified in NEEDS.

"Making AFTER/BEFORE not check for dependencies would constitute a breaking change." Indeed it does, but so long as you provide it optionally, then it is a non-issue. For example, you could add a MM config node option for patch config files. It would only be used for that specific .cfg and not relayed into the game.

MODULE_MANAGER { removeBeforeAfterDependency = true allowValueNameCharacters = () }

As far as internals are concerned, anything without removeBeforeAfterDependency would just have :AFTER[mod] become :NEEDS[mod]. Since anything specified in NEEDS must always load before the current patch. Currently, :BEFORE[mod] is basically useless because it constitutes a dependency while also asking load before its own dependency (which makes no sense).

As far as #96 preventing the zzzz race, I think you over-estimate people. People will just switch from using FOR to LAST and do the same thing all over again because that is exactly why they prefixed FOR with z's in the first place: to make sure it happened last. i.e. LAST[VSRPathPatch], LAST[zVSRVariants], LAST[zzVSRTU]

Ideally, I'd prefer a useLegacyBeforeAfter and default to non-dependency, but since that would require a lot work and no-longer-maintained mods would likely break, I don't see that as feasible.

It also occurs to me that FOR should be non-exclusive to others, or perhaps a different mechanism used to be able to name a patch file while still being able to control order.

Electrocutor commented 6 years ago

Key Points

FOR specifies the patch name and determines alphabetical order within other identical constraints
- f.e. :FOR[a]:AFTER[z] would happen before :FOR[b]:AFTER[z]
- f.e. :FOR[a]:AFTER[z] would happen after :FOR[b]:AFTER[z]:BEFORE[a]
- f.e. :FOR[a]:AFTER[z,c] would happen before :FOR[b]:AFTER[z] (because c is not loaded, thus ignored)
- f.e. :FOR[a]:FIRST would happen before :FOR[b]:FIRST
BEFORE and AFTER specifies 1+ non-dependency identifiers to determine order
NEEDS specifies 1+ dependencies
Any NEEDS identifiers are assumed as AFTER as well
FINAL reserved for personal patches
FIRST is exclusive to BEFORE, AFTER, and FINAL
FINAL is exclusive to FIRST, BEFORE, and AFTER

This is just a fleshed-out way of stating the initial ER.

JonnyOThan commented 4 years ago

in my dream world:

AFTER/BEFORE just sets up ordering links (and you can have multiple)
FOR associates the patch with a specific mod name (and the default just comes from the mod folder if not specified)
AFTER/BEFORE does not imply NEEDS (if the target mod does not exist, the ordering is not constrained). However this is a big breaking change, so maybe there needs to be a new keyword introduced.

Then once you have all the links set up, you just do a topological sort to get an order, and use alphabetic ordering to resolve ties. But you'd need some way to break cycles if someone declared an ordering loop.

I'm not really sure how LAST/FINAL/FIRST/LEGACY fit into this, but maybe you just do the whole above process for each of those passes?

7ranceaddic7 commented 4 years ago

FIRST is, well, first. As it implies, those patches are applied before all others.

Legacy, patches with no pass ordering, are applied second. (Personally, I think this functionality needs to be deprecated.)

BEFORE / FOR / AFTER are applied 3rd / 4th / 5th, respectively. FOR also functions as a declarative ("I Exist") for non-dll mods. (i.e. :FOR[TranceaddicT]).

LAST, is intended to be the final patch pass for mods themselves.

FINAL is intended to be for use by end users making their own modifications to their own installs.

FINAL should only be used by mods in one-off use-cases where it is the only available solution and an absolute necessity. (It is after all ... final.)

Each of these (even FIRST, FINAL, and legacy) is constrained by alphnumeric ordering.

JonnyOThan commented 4 years ago

BEFORE / FOR / AFTER are applied 3rd / 4th / 5th, respectively.

This isn't really true. Some patches marked "BEFORE" will occur after other patches marked "AFTER". This page explains it: https://github.com/sarbian/ModuleManager/wiki/Patch-Ordering

sarbian / ModuleManager

[Enhancement Request] Separate Dependency from Order #104

1 it would prevent the chaos that is every modder trying to come up with the correct name for FOR to make sure their patch ends up in the right place.