BUTR / MnB2-Bannerlord-CommunityPatch

This is going to be a mod that just fixes up some things in Mount & Blade 2: Bannerlord before the Devs & QA team can get to them. They have priorities and a process.
MIT License
88 stars 30 forks source link

Disabling patches when harmony collision detected, even if other mod is compatible. #183

Open RadthorDax opened 4 years ago

RadthorDax commented 4 years ago

As more and more methods are patched by the community mod, an increasing number of other mods will also be patching those methods, not always to fix the same issue as you guys are. Often these patches will even be compatible with the community patch. But when you disable your fixes on a hair trigger, it artificially creates a conflict where none exists.

I'd like to see some solution that allows the community patch to run alongside other mods that affect the same methods but play nice with each other. Whether that's a specific substring in the harmony patch id, or a prompt for the user to decide manually and the decision result saved, or something else entirely, I don't mind.

This is only going to become a larger issue as more mods get made and more fixes are added to the community patch.

DoctorVanGogh commented 4 years ago
_Tyler-IN: Collapsed rambling belligerent criticism including false allegations warranting a disrespectful censoring edit. There's some good advice here, but it's dripping with belligerence, and is ignorant of what is actually occurring in this mod._

There is _zero_ reason to reinvent the wheel here. Harmony allows _any_ number of patches to the same method. The problem here are **modders** (including **this** mod's authors) who decide to only go the "I'll prefix/postfix this method & replace the whole stinkin thing with _my_ logic and that's that. Screw everybody else!". This ultimately is not one iota better than just detouring stuff and _praying_ that you are the last guy who gets to detour everybody elses detours... *That is not a tennable situation*. --- Coming from modding a game (Rimworld) which did go through quite the same growing pains (everybody under the sun wanted to do *their* patch and did not think about playing well with others) I can tell that Harmony **used correctly** solves all those problems. That game now enjoys thousands and thousands of different mods, with an overwhelming amount of mods (I'd say >99,9%) not having any undesired interactions or blocking each other. That is because someone put together a tool which allows multiple people mucking around in the same functionality without everybody blocking everybody else. Problem is: *That tool needs to be used _correctly_*. Which in Harmony's case means: nine times out of ten people need to either * use Postfixes to let default logic run and then muck around with the returned results as desired * or use a `HarmonyTranspiler` to surgically only modify _that_ tiny part of the default method which deals with the functionality you want to change and not just make a copy and wrench in your own stuff. I'm aware that writing transpilers - especially transpilers which interact nice with potential other transpilers - is **hard**. Hard as in "it might not be rocket science, but you certainly can at least see that topic from where you're at". _Of course_ there will always be collisions which can not be resolved. If mod `Foo` wants to change a skill to give an additional `30%` bonus it might just end up multiplying the default value with `1.3`. Now if another mod is already in there and adds its own `20%` bonus you might end up with `50% (30+20)` or even `56%` bonus (1.3*1.2). That's something the user is going to have to live with, because if they run multiple things which modify the very same thing there may well be interactions. But *please* dont go "lalala, someone touched this I'm not doing my stuff" or the reverse "screw everybody else, I'll just jam in _my_ prefix". Write a postfix... write a transpiler... write _minimal_ patches!

Tyler-IN commented 4 years ago
Response to DoctorVanGogh.

> The problem here are **modders** (including **this** mod's authors) who decide to only go the "I'll prefix/postfix this method & replace the whole stinkin thing with _my_ logic and that's that. Screw everybody else!". Hey stop being belligerent. Our patches look like this, from [NeutralClanTierUpPatch.cs](https://github.com/Tyler-IN/MnB2-Bannerlord-CommunityPatch/blob/dev/src/CommunityPatch/Patches/NeutralClanTierUpPatch.cs) as an example; ```cs private static readonly MethodInfo TargetMethodInfo = typeof(HeroSpawnCampaignBehavior).GetMethod("OnClanTierIncreased", BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.DeclaredOnly); private static readonly MethodInfo PatchMethodInfo = typeof(NeutralClanTierUpPatch).GetMethod(nameof(Prefix), BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.DeclaredOnly); public override IEnumerable GetMethodsChecked() { yield return TargetMethodInfo; } private static readonly byte[][] Hashes = { new byte[] { // e1.1.0.224785 0x6C, 0x46, 0xC7, 0xAA, 0x16, 0x02, 0x77, 0xF2, 0x90, 0x37, 0xE4, 0x9B, 0x8D, 0x25, 0x46, 0x96, 0x6C, 0xA3, 0x9D, 0xAB, 0x77, 0x3D, 0x6E, 0x28, 0x8F, 0x5B, 0x4F, 0xB1, 0xA9, 0xFF, 0xB1, 0xAA } }; public override void Reset() { } public override bool? IsApplicable(Game game) { var patchInfo = Harmony.GetPatchInfo(TargetMethodInfo); if (AlreadyPatchedByOthers(patchInfo)) return false; var hash = TargetMethodInfo.MakeCilSignatureSha256(); if (!hash.MatchesAnySha256(Hashes)) return false; try { if (CampaignData.NeutralFaction.DefaultPartyTemplate != null) return false; } catch (Exception) { return null; } return true; } public override void Apply(Game game) { if (Applied) return; CommunityPatchSubModule.Harmony.Patch(TargetMethodInfo, prefix: new HarmonyMethod(PatchMethodInfo)); Applied = true; } // ReSharper disable once InconsistentNaming [MethodImpl(MethodImplOptions.NoInlining)] private static bool Prefix(Clan clan) { if (clan == CampaignData.NeutralFaction) return false; return true; } ``` Our patches are conservative, they don't patch if someone else has patched it first, or if the underlying IL has changed. We apply our patches late. If someone loads patches later than us, our patches can be patched over and stack (see: `return true`). Some method logic simply has to be discarded if the entire result is based on one factor (`OnClanTierIncreased` should just not do anything for neutral clans, the original code throws) or another. We could have used a Finalizer here to discard the exception, and in the future we might, and you didn't mention those. We also use Postfixes where needed and Transpilers where we have to. We're a community effort, and our patch authors have differing levels of skill, differing opinions, and we're working together to do a good job. You can help by creating bug reports or submitting fixes or enhancements by PRs. You can criticize if you want, but attempting raising some air of superiority, getting on some high horse of arrogance, is not acceptable. This belligerence is foolishness.

Tyler-IN commented 4 years ago

But when you disable your fixes on a hair trigger, it artificially creates a conflict where none exists.

We intend to have options to be added in a GUI that gives each patch 3 states; Automatic, Forced, and Disabled. Automatic is the current mode of operation.

It should also show the applicability of the patch, whether or not it has been applied, and other information.

Right now this information is only visible in a diagnostic report.