How do you modify named constructors?

[gregros]

How do you modify named constructors?

[GregRos]

Glad to see some interested in this project 😄

Here is some info about modifying constructors:
https://github.com/GregRos/Patchwork#modifying-constructors

[fireundubh]

For constructors with parameters, I've never had any luck with this pattern. The launcher fails with a cannot find key in dictionary exception.

[MemberAlias(".ctor", typeof(object))]
private void object_ctor()
{
	// this is an alias for object::.ctor()
}

[ModifiesMember(".ctor")]
public void CtorNew(UnitDescriptor unit, bool isPregen)
{
	this.object_ctor();
}

But if I duplicate the constructor and call it in the new constructor, the patcher succeeds:

[MemberAlias(".ctor", typeof(object))]
private void object_ctor()
{
	// this is an alias for object::.ctor()
}

[NewMember]
[DuplicatesBody(".ctor")]
public void source_ctor(UnitDescriptor unit, bool isPregen)
{
	throw new DeadEndException("source_ctor");
}

[ModifiesMember(".ctor")]
public void CtorNew(UnitDescriptor unit, bool isPregen)
{
	this.object_ctor();
	this.source_ctor();
}

That pattern has some limitations though because you can't directly modify anything in the duplicated body.

Also, some methods require an instance constructor like:

public LevelUpStateNew(UnitDescriptor unit, bool isPregen) : base(unit, isPregen)
{
}

I figured out you can just leave the body empty. I mark that constructor with the [Obsolete] attribute.

[GregRos]

The probable issue is that there is some important code in the original constructor that you need to duplicate in your modification, though I can't imagine how it would make the launcher crash.

The instance constructor includes, among other things, the field/property initializers.

It sounds like the class you're modifying a class with a base class of its own. You will need to call base::.ctor() instead of object::.ctor, because the base class is probably not object.

[fireundubh]

The probable issue is that there is some important code in the original constructor that you need to duplicate in your modification, though I can't imagine how it would make the launcher crash.

Well, I tried the MemberAlias/ModifiesMember .ctor pattern again. No issues. I'm guessing I was having trouble before because I was trying to do something with the instance constructor that the compiler requires.

It sounds like the class you're modifying a class with a base class of its own. You will need to call base::.ctor() instead of object::.ctor, because the base class is probably not object.

I don't see a base class in the decompiled source. For example:

public class LevelUpState {
	public LevelUpState(UnitDescriptor unit, bool isPregen) {
		// snip
	}
}

In my mod, I create a new class and inherit from the original class:

[ModifiesType]
public class LevelUpStateNew : LevelUpState
{ 
	// snip
}

dotPeek shows this:

  // .class public auto ansi beforefieldinit 
    // Kingmaker.UnitLogic.Class.LevelUp.LevelUpState
      // extends [mscorlib]System.Object

I don't know why an instance constructor is required for this modifying class and not others, but VS complains, so I generate the instance constructor:

[ModifiesType]
public class LevelUpStateNew : LevelUpState
{ 
	[Obsolete]
	public LevelUpStateNew(UnitDescriptor unit, bool isPregen) : base(unit, isPregen)
	{
	}
}

I just leave it empty, and since there's no Patchwork attribute, it doesn't get patched in.

If it gets patched in like this:

[ModifiesMember("LevelUpState")]
public LevelUpStateNew(UnitDescriptor unit, bool isPregen) : base(unit, isPregen)
{
}

Patchwork throws a dictionary exception.:

PatchworkLauncher.PatchingProcessException
  HResult=0x80131500
  Message=Exception of type 'PatchworkLauncher.PatchingProcessException' was thrown.
  Source=PatchworkLauncher
  StackTrace:
   at PatchworkLauncher.LaunchManager.ApplyInstructions(IEnumerable`1 patchGroups, ProgressObject po) in E:\projects\PatchworkPathfinder\PatchworkLauncher\GUI\LaunchManager.cs:line 749
   at PatchworkLauncher.LaunchManager.<>c__DisplayClass46_1.<Command_Patch>b__0() in E:\projects\PatchworkPathfinder\PatchworkLauncher\GUI\LaunchManager.cs:line 369
   at System.Threading.Tasks.Task.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()

Inner Exception 1:
PatchDeclerationException: The attribute on the method called 'System.Void KingmakerMods.Mods.CustomPointBuy.LevelUpStateNew::.ctor(Kingmaker.UnitLogic.UnitDescriptor,System.Boolean)' refers to 'LevelUpState', but that member doesn't exist in this context (possibly overload resolution failure).

If there's a non-empty constructor, then I use one of the .ctor patterns we talked about.

The only time I don't inherit from the original class is when I'm modifying a sealed class - because, well, you can't do that, but you can specify the full path to the class in [ModifiesType] like:

  // .class public abstract sealed auto ansi beforefieldinit 
    // Kingmaker.UI.Common.StatModifiersBreakdown
      // extends [mscorlib]System.Object

[ModifiesType("Kingmaker.UI.Common.StatModifiersBreakdown")]
public static class StatModifiersBreakdownNew { // snip }

[fireundubh]

Well, I tried the MemberAlias/ModifiesMember .ctor pattern again. No issues.

Spoke too soon. 😢

[GregRos]

I don't know why an instance constructor is required for this modifying class and not others, but VS complains, so I generate the instance constructor:

That's because in C#, when you use inheritance, your subclass constructor must call a superclass constructor. If there is no default constructor in the superclass, you must write the constructor call yourself with the right parameters, etc.

You can avoid the issue if you just use [ModifiesType] with a class name and avoid using inheritance (this will introduce a few other issues though).

I just leave it empty, and since there's no Patchwork attribute, it doesn't get patched in.

Yup, that can also work.

Patchwork throws a dictionary exception.:

Makes perfect sense, because there is no member called LevelUpState. The constructor is just called .ctor. There is no such thing in C# or IL as a named constructor.

Spoke too soon. 😢

Does that mean you're still having problems?

[fireundubh]

You can avoid the issue if you just use [ModifiesType] with a class name and avoid using inheritance (this will introduce a few other issues though).

What issues?

Makes perfect sense, because there is no member called LevelUpState. The constructor is just called .ctor. There is no such thing in C# or IL as a named constructor.

Yeah, that was me expecting [ModifiesMember(".ctor")] on LevelUpStateNew to handle it. 😄

Does that mean you're still having problems?

If I use MemberAlias/ModifiesMember, Patchwork verification completes without errors but the game freezes.

[MemberAlias(".ctor", typeof(object))]
private void object_ctor()
{
}

[ModifiesMember(".ctor")]
public void mod_ctor()
{
	this.object_ctor();

	// modified ctor body
}

If I use MemberAlias/NewMember/ModifiesMember:

[MemberAlias(".ctor", typeof(object))]
private void object_ctor()
{
}

[NewMember]
[DuplicatesBody(".ctor")]
public void source_ctor()
{
	throw new DeadEndException("source_ctor");
}

[ModifiesMember(".ctor")]
public void mod_ctor()
{
	this.object_ctor();
	this.source_ctor();

	// just variables
}

I see these errors during Patchwork verification, but the game does not freeze and the mods work:

[IL]: Error: [Assembly-CSharp : Kingmaker.UnitLogic.Class.LevelUp.StatsDistribution::source_ctor][offset 0x0000000C][found ref ('this' ptr) 'Kingmaker.UnitLogic.Class.LevelUp.StatsDistribution'] Call to .ctor only allowed to initialize this pointer from within a .ctor. Try newobj.(Error: 0x801318BF)
[IL]: Error: [Assembly-CSharp : Kingmaker.UnitLogic.Class.LevelUp.LevelUpState::source_ctor][offset 0x00000006] Cannot change initonly field outside its .ctor.(Error: 0x80131884)
2 Error(s) Verifying Assembly-CSharp

Glad to see some interested in this project

By the way, there are a few Patchwork projects out there: IEMod/IEMod.pw, of course, but also SonicZentropy/PoE2Mods.pw, SonicZentropy/TyrannyMod.pw, and llerelol/patchwork-mods-tyranny. My project is fireundubh/KingmakerMods.pw for Pathfinder: Kingmaker.

[GregRos]

What issues?

Calling other instance methods on the modified type will be awkward.

If I use MemberAlias/ModifiesMember, Patchwork verification completes without errors but the game freezes.
If I use MemberAlias/NewMember/ModifiesMember:
I see these errors during Patchwork verification, but the game does not freeze and the mods work:

Once again, this is probably caused by there being some important operations in the constructor that you're not copying to your modified constructor.

The issue with DuplicatesBody is that you're putting commands from a ctor inside a non-ctor method. Some of those commands are illegal in that context. Don't use it like this.

What you should do is decompile the code of the class, look at both the commands in the constructor and any initializers, and put them in your ctor.

By the way, there are a few Patchwork projects out there: IEMod/IEMod.pw, of course, but also SonicZentropy/PoE2Mods.pw, SonicZentropy/TyrannyMod.pw, and llerelol/patchwork-mods-tyranny. My project is fireundubh/KingmakerMods.pw for Pathfinder: Kingmaker.

That's great! I'm happy the framework is seeing use. Maybe I should go back to developing it.

[fireundubh]

Once again, this is probably caused by there being some important operations in the constructor that you're not copying to your modified constructor.
What you should do is decompile the code of the class, look at both the commands in the constructor and any initializers, and put them in your ctor.

Everything that's in the source constructor is in the modified constructor. That's why I'm confused by the behavior.

Here's the code and debug log: https://gist.github.com/fireundubh/dd7cebc0b45c0af591b435a1a8199677

[GregRos]

@fireundubh Can you show me the source code of the class you're working on? Send me both IL and C# output if you can.

[fireundubh]

@fireundubh Can you show me the source code of the class you're working on? Send me both IL and C# output if you can.

Here you go. This is output from dotPeek:
https://gist.github.com/fireundubh/0d3ae122a6e591dc851ab34b8c37f46a

Here's a version with compiler-generated code and tokens:
https://gist.github.com/fireundubh/e0bc5ccd3b7a1b6e2cac47ce27f111fd

[GregRos]

@fireundubh Yeah, I didn't see anything odd there. Can you send me your project? Or maybe push to github into a separate branch and I'll take a look at it.

If it comes to that, I'll set up a development environment, find the game, and try to debug the patching process.

[fireundubh]

The project is on GitHub at the link I mentioned. Not sure what else I can provide?

In any case, I updated PEVerify, but the same error occurred, so I started refactoring. I tried just NewMember/DuplicatesBody for the .ctor without calling it anywhere, and PEVerify produced the same error. PEVerify doesn't seem to like when the .ctor is duplicated. Thing is, the mod works and I know IEMod does this, so is the IL error a problem? I see in the available flags for PEVerify that it's possible to ignore certain errors, so maybe PEVerify is overly strict by default?

[GregRos]

@fireundubh The problem is not in PEVerify. PEVerify is totally correct. As I've said previously, injecting code from a constructor into a non-constructor method is a Bad Thing. You are not supposed to do that.

Now, it is also true that not every issue PEVerify tells you about will actually crash the game. Some errors are actually totally harmless and others will cause the platform to just skip over instructions that don't make sense.

The errors you have aren't harmless, but maybe the platform will skip over the illegal commands and nothing will happen. You CAN ignore them if you like, but it's not a good idea, since they DO indicate definite issues in the code.

The actual problem here is that:

If I use MemberAlias/ModifiesMember, Patchwork verification completes without errors but the game freezes.

It is clear that some instructions that are supposed to be in the constructor aren't actually ending up there, or maybe something else is screwing things up.

This might be caused by your code, or it might be a bug in Patchwork. I should have some time on the weekend to look at it more.

Edit

I looked at the code you sent, and you actually sent me StatsDistribution, but you're really working on LevelUpState...... can you send me the code for that class?

[fireundubh]

I figured out the problem. It was like you said: some calls were missing from the new .ctor.

When dnSpy decompiles, it sometimes assigns fields in the class:

public class LevelUpState
{
	[NotNull]
	public readonly List<FeatureSelectionState> Selections = new List<FeatureSelectionState>();

	[NotNull]
	public readonly List<SpellSelectionData> SpellSelections = new List<SpellSelectionData>();

	[NotNull]
	public AlignmentRestriction AlignmentRestriction = new AlignmentRestriction();

	[NotNull]
	public StatsDistribution StatsDistribution = new StatsDistribution();

When dotPeek decompiles (with "show compiler-generated code" enabled), it shows those fields assigned in the .ctor:

public LevelUpState(UnitDescriptor unit, bool isPregen)
{
	this.Selections = new List<FeatureSelectionState>();
	this.SpellSelections = new List<SpellSelectionData>();
	this.AlignmentRestriction = new AlignmentRestriction();
	this.StatsDistribution = new StatsDistribution();
	base.\u002Ector();

Once I assigned those fields in the new .ctor, the game no longer crashed when I used the MemberAlias/ModifiesMember pattern for the new .ctor.

[GregRos]

@fireundubh That's great!

Yup, those are called field initializers. They look and act different in C#, but they're just compiled to regular instructions in the constructor. Patchwork can't tell them apart from regular constructor instructions (neither can some decompilers), but it IS possible in most cases, so some decompilers do this.

I tried to search for them in the code you sent me, but because you sent me a different class, I couldn't find any.

[fireundubh]

FYI: dnSpy/dnSpy@5deaf1e

dnSpy was missing the checkbox, so now the latest version allows you to toggle where field initializers are shown in the decomp.