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

Adds support for targeting an async method's internal MoveNext method #514

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

Banane9
Copy link
Contributor

@Banane9 Banane9 commented Mar 2, 2023

As the title says. This is particularly interesting for targeting an async method with a transpiler, whether using Attributes or manually finding it with the added AsyncMoveNext method in AccessTools.

I'm not sure where the tests should go / how they should look, but I did test it on a game that I write mods for.
Works perfectly, although trying to use IAsyncStateMachine as the type for the __instance parameter causes a FormatException for invalid IL on the call to the patch method (that took a while to figure out).

Definition of Patches

Resulting output in log showing all patches are successfully applied

… MoveNext with Attributes

Co-authored-by: Michael Ripley <zkxs00@gmail.com>
@pardeike
Copy link
Owner

pardeike commented Mar 2, 2023

What’s the difference to MethodType.Enumerator ?

@Banane9
Copy link
Contributor Author

Banane9 commented Mar 2, 2023

Well, with MethodType.Enumerator it fails to find the method to patch, as it looks for a newobj opcode - but the actual contents of async methods don't use that.

Looking at an iterator method, those are also decorated with an IteratorStateMachineAttribute, which contains a reference to the type. They both share the StateMachineAttribute as a parent, so I suppose it could be merged together into a StateMachine one too (though that wouldn't be backwards compatible, I guess).

@Banane9
Copy link
Contributor Author

Banane9 commented Mar 20, 2023

Anything that would have to be changed/added for this? 👀

A game I'm working with has quite a few async methods, so it would be nice to target them more easily.

@pardeike
Copy link
Owner

I would still like to see if we can merge this with the existing api. You mentioned that this would break backwards compatibility, in what way would that be a problem?

@Banane9
Copy link
Contributor Author

Banane9 commented Mar 21, 2023

I just mentioned it as a consideration - I don't know how you handle that sort of thing for Harmony.

Funnily enough, the way I understand it, renaming the MethodType.Enumerator enum value to something like MethodType.StateMachine and handling both through that would keep it binary compatible (as enum values are embedded directly as their numeric values), but code using it would have to be changed.

Though even that could be avoided by defining it as a second entry in the enum with the same underlying value and marking the old one as obsolete / removing it with a major version release - though I don't know if you care about keeping that sort of compatibility.

Copy link
Owner

@pardeike pardeike left a comment

Choose a reason for hiding this comment

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

Just that one tiny cleanup

Harmony/Public/Attributes.cs Outdated Show resolved Hide resolved
@pardeike pardeike merged commit 4e50067 into pardeike:master Mar 22, 2023
Windows10CE added a commit to Windows10CE/HarmonyX that referenced this pull request Jul 2, 2023
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.

2 participants