-
Notifications
You must be signed in to change notification settings - Fork 42
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
In Dyson Sphere Program 0.7.x targeting multiple methods not functional? #27
Comments
Did a bit of testing, and can confirm this is an issue in latest HarmonyX with a minimal example DSPMultiplePatchTest.zip [HarmonyPatch(typeof(UIEscMenu))]
public static class PatchesFail
{
[HarmonyPrefix]
[HarmonyPatch(nameof(UIEscMenu.OnButton5Click))]
[HarmonyPatch(nameof(UIEscMenu.OnButton6Click))]
public static void TestPrefix(MethodBase __originalMethod)
{
Class1.Logger.LogMessage($"This should fail for Quit Game (5) or Exit to Desktop (6): {__originalMethod.Name}");
}
}
[HarmonyPatch]
public static class PatchesSucceed
{
[HarmonyPrefix]
[HarmonyPatch(typeof(UIEscMenu), nameof(UIEscMenu.OnButton5Click))]
[HarmonyPatch(typeof(UIEscMenu), nameof(UIEscMenu.OnButton6Click))]
public static void TestPrefix(MethodBase __originalMethod)
{
Class1.Logger.LogMessage($"This should succeed for Quit Game (5) and Exit to Desktop (6): {__originalMethod.Name}");
}
} The top fails, the bottom succeeds. This leads me to believe the error is somewhere in attribute merging, but I haven't gotten to narrowing it down yet. |
Alright, I've managed to track down where this came from, although I'm not entirely sure whether or not it's an actual bug or just poorly documented. The behavior comes from this line, where it confirms that each attribute is "complete" as defined by https://github.com/BepInEx/HarmonyX/wiki/Multitargeted-patches . HarmonyX/Harmony/Internal/PatchModels.cs Line 123 in c0f6004
I'm not sure how this worked in earlier versions of DSP, maybe it's been broken the whole time and just didn't throw an NRE previously? Whether or not it's a bug to try and be fixed is up to @ghorsington rather than me, if it's to be left as is I'll probably add more emphasis to the multi-patch wiki page about each attribute needing to be complete on it's own, with nothing coming from attributes on the patch class. |
It definitely runs in 0.6.x, I just double-checked by throwing a log statement in there and it prints as expected. If this isn't intended behavior it's fine then, but I think the format makes more sense than having to specify the type in each line and it worked well up until now. |
The top version working was unintended I believe, the assumption always was that you provide all information in single attributes when multi-targeting. |
Just reporting back because I didn't realize where that format came from, but it's actually from Harmony's documentation: |
The documentation is for the per-assembly patching method which is used in stock harmony, not per-type patching which is unique to harmonyx. |
Going to confirm here: the multitargeting syntax @Windows10CE shows in The syntax shown in the Nevertheless, I don't see why one couldn't allow multitargeting in class patches provided that at least one of the two holds:
I'll add this feature, test it and close the issue once it's fixed. |
This code works fine on Dyson Sphere Program 0.6.x (tested with BepInEx 5.4.5 & 5.4.9), however on 0.7.x with the 2 aforementioned versions of BepInEx and also 5.4.11 this code never runs. It works fine if each patch only targets each of the methods separately.
Is this something that can be/has to be fixed within HarmonyX?
The text was updated successfully, but these errors were encountered: