Vanilla-Expanded / VanillaExpandedFramework

Vanilla Expanded Framework for RimWorld
Other
74 stars 37 forks source link

HumanlikeMeshPoolUtility Transpilers disregard state #15

Closed oorzkws closed 2 years ago

oorzkws commented 2 years ago

This is mainly a compatibility issue, in that the VEF patches will do its own calculation without considering any variables already present in the functions. For example, pawn.ageTracker.CurLifeStage.bodyWidth may be referenced in a function, but VEF instead takes it off the stack and then calls its own function with just pawn. This is an issue if another mod changes the value within the stack.

This pattern is often repeated between the various patches to HumanlikeMeshPoolUtility and PawnRenderer.

I literally learned transpilers just for this issue, so take any suggestions with the caveat that I'm entirely inexperienced - I would like your feedback on the optimal solution

original:

public static GraphicMeshSet GetHumanlikeBodySetForPawn(Pawn pawn)
{
    if (ModsConfig.BiotechActive && pawn.ageTracker.CurLifeStage.bodyWidth != null)
    {
        return MeshPool.GetMeshSetForWidth(pawn.ageTracker.CurLifeStage.bodyWidth.Value);
    }
    return MeshPool.humanlikeBodySet;
}

With current VEF (approximation)

public static GraphicMeshSet GetHumanlikeBodySetForPawn(Pawn pawn)
{
    if (ModsConfig.BiotechActive && pawn.ageTracker.CurLifeStage.bodyWidth != null)
    {
        // Not actually a local, just showing the discard process
        var MeshSet = MeshPool.GetMeshSetForWidth(pawn.ageTracker.CurLifeStage.bodyWidth.Value);
        return updatedMeshSet(pawn);
    }
    return updatedMeshSet(pawn);
}

This can result in situations like the below when VEF and other mod patches collide

public static GraphicMeshSet GetHumanlikeBodySetForPawn(Pawn pawn)
{
    var width = HumanlikeBodyWidthForPawn(pawn); // Not actually a local, just representing the stack
    width *= someOtherModFunction(pawn); // Also could be width = someOtherModFunction(pawn, width)
    if (ModsConfig.BiotechActive && pawn.ageTracker.CurLifeStage.bodyWidth != null)
    {
        // return MeshPool.GetMeshSetForWidth(width); // Before VEF change
        return updatedMeshSet(pawn); // with VEF change, stack is discarded
    }
    // return MeshPool.GetMeshSetForWidth(width); // Before VEF change
    return updatedMeshSet(pawn); // with VEF change, stack is discarded
}

Ideal:

public static GraphicMeshSet GetHumanlikeBodySetForPawn(Pawn pawn)
{
    var pawnScale = HumanlikeMeshPoolUtility_Patches.GeneScaleFactor(pawn, HumanlikeBodyWidthForPawn(pawn)); // Not a local, representing stack
    if (ModsConfig.BiotechActive && pawn.ageTracker.CurLifeStage.bodyWidth != null)
    {
        return MeshPool.GetMeshSetForWidth(pawnScale);
    }
    return MeshPool.GetMeshSetForWidth(pawnScale);
}

Alternative ideal, a heavier change but this is calling a builtin function (HumanlikeMeshPoolUtility.HumanlikeBodyWidthForPawn) that can then just be postfixed for better patch intercompatibility. I'm not sure if this is a good idea considering the signatures/pattern matches it would break, though.

public static GraphicMeshSet GetHumanlikeBodySetForPawn(Pawn pawn)
{
    return MeshPool.GetMeshSetForWidth(HumanlikeBodyWidthForPawn(pawn));
}
ANRimworld commented 2 years ago

Hey! Ah yeah I was grabbing the factor at one point but got lazy during one of the changes looks like.

I have made changes that should be merged to main soon that will make this easier. I both grab off the stack, but I have also implemented a call to that utility method. Since it's not used by Ludeon at all, it only gets called via our patches.

This branch has the changes

oorzkws commented 2 years ago

Hey! Ah yeah I was grabbing the factor at one point but got lazy during one of the changes looks like.

I have made changes that should be merged to main soon that will make this easier. I both grab off the stack, but I have also implemented a call to that utility method. Since it's not used by Ludeon at all, it only gets called via our patches.

This branch has the changes

Thanks for giving this your attention, I knew it was a minor issue at best when I filed it. That implementation looks good, I should be able to maintain compatibility now without trying to re-implement the entire VEF logic on my side.

oorzkws commented 2 years ago

A few hours later I've integrated your changes. Looks like there's a few other functions I would need to patch as-is, thoughts? edit: GetHumanlikeHairSetForPawn, GetHumanlikeBeardSetForPawn, BaseHeadOffsetAt, maybe DrawBodyGenes and DrawExtraEyeGraphic

-2 through 2 have my body size genes, 3 has my largest + the alpha larger gene, -3 has my smallest plus the alpha smaller gene.

image

Taranchuk commented 2 years ago

Can you check the current VEF version and see if it fixed your issue?

oorzkws commented 2 years ago

Can you check the current VEF version and see if it fixed your issue?

Head offset seems wrong on the smallest body size, and mixing my body size genes with the alpha genes doesn't seem to have an effect for the smaller - one seems to override the other.

My mod scaling through VEF (patching BodySizeWidthForPawn), the "mixed" pawns also have the AG body size genes. image

Second image is just with my mod's patches (VFE methods unpatched, doesn't account for AG scaling) image

Can I ask why the method for the AG genes is to manually parse the genes every time when you could introduce a stat def (example, example gene) which would let you aggregate the stat from multiple sources/mods with a single call?

ANRimworld commented 2 years ago

So beard/hair uses headSizeFactor. Not HumanlikeBodyWidthForPawn so it wont work the same I'm afraid. You'd have to either patch your factors into the GetHumanlikeHairSetForPawn and GetHumanlikeBeardSetForPawn methods. Or you can just use the framework for it, so you don't need to make your own transpilers. It is pushed to steam now.

As for why its not a Statdef. Kind of 2 reasons 1 A def extension is just easiest option for a framework. Anyone with a dependency can easily add it with no risk of conflict or issues.

  1. A stat is not a vector2. Would need to make 4 statdefs, and build the body and head vector2s. I doubt there'd be any performance gains doing it the stat method, plus would be pretty preemptive.

If you want to just go the route of using the framework for your mod. This is the AG implementation. You'd just need to add framework as a dependency.

oorzkws commented 2 years ago

So beard/hair uses headSizeFactor. Not HumanlikeBodyWidthForPawn so it wont work the same I'm afraid. You'd have to either patch your factors into the GetHumanlikeHairSetForPawn and GetHumanlikeBeardSetForPawn methods. Or you can just use the framework for it, so you don't need to make your own transpilers. It is pushed to steam now.

Thanks, I'd rather not have to depend on an outside framework. I have the transpilers worked out, but was hoping to avoid having to reproduce the VEF logic on my side to keep compatibility. I'll go ahead with that plan now, hopefully by calling LifeStageFactorUpdated via reflection I can avoid too many breakages if the logic changes in VEF.