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

Merging upstream #97

Merged
merged 210 commits into from
May 30, 2024
Merged

Merging upstream #97

merged 210 commits into from
May 30, 2024

Conversation

kohanis
Copy link
Contributor

@kohanis kohanis commented Dec 18, 2023

Merging up to v2.3.1.1 of upstream + some fixes

simplyWiri and others added 30 commits January 8, 2022 20:08
… in HarmonySharedState.

- Bump the internal version field in HarmonySharedState
Use complex LambdaExpression walking to allow  GetMethodInfo (() => myMethod) rather than GetMethodInfo(() => myMethod(null,null,null....))
…decoding_pr

Ok, after consideration, especially with the fact that you need the latest language version, I decided that the risk is minimal. I will just take it as it is since less experienced users are most likely to affected due to them not running the latest language features.
They are supported when just inspecting the IL code
mentions that filter blocks are unsupported in the documentation
Improve Performance of `FindReplacement`
@CptMoore
Copy link

Looks like @kohanis fixes to native detour patcher indeed fixes issue #107, thanks.
Could you also use
<MonoModRuntimeDetour>25.1.0</MonoModRuntimeDetour>
instead of the prelease version, as it includes a vital unity+linux fix.

@kohanis
Copy link
Contributor Author

kohanis commented Mar 14, 2024

That indeed did the trick. Though why is that required now? It was working fine in the previous version of Harmony (and it breaks existing patches).

Frankly I don't see any reason why mono wouldn't inline a purely throwing method without [MethodImpl(MethodImplOptions.NoInlining)].
Is the usage exactly the same? If method with reverse patch call is called at least once before patching itself, this behaviour is not surprising
// But the fact that it is called before patching is, imho, misuse of it

@Meivyn
Copy link
Contributor

Meivyn commented Mar 14, 2024

Watch out, bumping the MonoMod.RuntimeDetour version requires Harmony to target net452 instead of net45, otherwise it won't use the correct version of Mono.Cecil.

Is the usage exactly the same? If method with reverse patch call is called at least once before patching itself, this behaviour is not surprising

Yes, the usage is the same, as I said it was breaking patches that did work before and it breaks my test one as well, and as you can see that method isn't called before patching. Patching happens in Init which is always called before OnEnable as it's the constructor.

[Plugin(RuntimeOptions.DynamicInit)]
public class Plugin
{
    internal static IPALogger Log { get; private set; } = null!;

    [Init]
    public Plugin(IPALogger logger, PluginMetadata metadata)
    {
        Log = logger;
        Harmony.CreateAndPatchAll(metadata.Assembly, "com.meivyn.BeatSaber.BSPlugin5");
    }

    [OnEnable]
    public void OnEnable()
    {
        Patch.ReversePatch();
    }

    [OnDisable]
    public void OnDisable()
    {
    }

    public void TestMethod()
    {
        Log.Notice(nameof(TestMethod));
    }
}


[HarmonyPatch(typeof(Plugin), nameof(Plugin.TestMethod))]
internal class Patch
{
    [HarmonyReversePatch]
    [MethodImpl(MethodImplOptions.NoInlining)] // breaks without this
    public static void ReversePatch()
    {
        throw new NotImplementedException("Reverse patch has not been executed.");

        IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions)
        {
            return new CodeMatcher(instructions)
                .MatchStartForward(new CodeMatch(OpCodes.Ldstr))
                .SetOperandAndAdvance("Reverse patched")
                .InstructionEnumeration();
        }
    }
}

@kohanis
Copy link
Contributor Author

kohanis commented Mar 14, 2024

Yes, the usage is the same, as I said it was breaking patches that did work before and it breaks my test one as well, and as you can see that method isn't called before patching. Patching happens in Init which is always called before OnEnable as it's the constructor.

Cannot confirm it. Used latest BSIPA 4.3.2 with your sample without MethodImplOptions.NoInlining on PlateUp, got NotImplementedException

@Meivyn
Copy link
Contributor

Meivyn commented Mar 14, 2024

Yeah, I actually cannot reproduce the same behavior with my patch as well. It seems to happen only to this patch: https://github.com/Aeroluna/Heck/blob/master/Heck/HarmonyPatches/PlayViewInterrupter.cs#L76, at least from my testing. Using Harmony 2.12.0 broke this patch, and it was not throwing before that. Reverting to Harmony 2.10.2 fixes it.

@kohanis
Copy link
Contributor Author

kohanis commented Mar 15, 2024

Okay, reverse patches are broken in general in 1.11.0+, affirmative. GC collects them. I'll see what I can come up with

@kohanis
Copy link
Contributor Author

kohanis commented Mar 15, 2024

Far from perfect, but let's call it a hotfix for now
// btw, it wasn't previously implemented correctly either, it's just that ILHook didn't have a finalizer previously

@Meivyn
Copy link
Contributor

Meivyn commented Mar 15, 2024

That doesn't fix the inline issue with my test patch, however it seems to have fixed the broken patch I mentioned earlier. Guess I'm only missing a fix for the IL error that happens when reverse patching some methods before using this PR in BSIPA.

I highly doubt those are the only bugs we are gonna encounter, but I guess it is usable enough. Thanks for your fixes.

@kohanis
Copy link
Contributor Author

kohanis commented Mar 15, 2024

Restructured. Last push in this PR, unless fixes related to it are needed. I'm planning some structural changes of project based on this PR, but as separate one

@ManlyMarco ManlyMarco requested review from js6pak and bbepis March 16, 2024 10:46
@kohanis
Copy link
Contributor Author

kohanis commented Mar 17, 2024

Pardon, that fix was important. Also, friendly reminder that without HarmonyX.Ref package project will not be built as is, and CI is also not adapted for this

@kohanis
Copy link
Contributor Author

kohanis commented Mar 18, 2024

As far as I can tell it should work the same way, but with two packages. Not sure about integration with jenkins though

@CptMoore
Copy link

whats missing for this to be merged? CI? HarmonyX.Ref?

Copy link
Member

@ManlyMarco ManlyMarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me so long to review, most of the changes are just syntax fluff which makes it hard to spot and review any material changes.

As far as I can see this is good to merge and shouldn't break binary backwards compatibility. There may be some minor source code breaks but that's fine.
It might require a major version bump still, but that's for later.

My main worry is workflows and dealing with the ref/full situation. Ideally builds and packages would stay in the same format as they are now, in my opinion at least.

If no one else raises any issues with this PR in the meantime I'll merge this later next week.

.github/workflows/publish_release.yml Show resolved Hide resolved
@kohanis
Copy link
Contributor Author

kohanis commented May 18, 2024

Theoretically I can merge netstandard references into main package, will check.

@kohanis
Copy link
Contributor Author

kohanis commented May 19, 2024

Okay, there's a reason for separate package (harmony's discord).
But, after rethinking, I realized that the restriction on netstandard does not apply to harmonyx. Harmony's problem with it is ilmerge.
So, this should work. I plan to later squish this into the first commit
EDIT: it might make sense to make separate target for netcoreapp3.(0/1) tho. To avoid dragging json

@ManlyMarco ManlyMarco merged commit 7c44c07 into BepInEx:master May 30, 2024
2 checks passed
@kohanis kohanis deleted the merge-2.2.2 branch May 30, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.