sumghai / MedPod

Advanced regenerative medical beds for RimWorld
Other
14 stars 15 forks source link

Patching For Elite Bionic Framework #22

Closed LastXsile closed 3 years ago

LastXsile commented 3 years ago

Patching to remove the error message. However this doesn't prevent it from working properly so I myself would consider this a low priority fix.

[V1024-EBF] Elite Bionics Framework has detected some mods using the unmodified GetMaxHealth() method, which violates the EBF protocol. The author(s) of the involved mod(s) should adopt the EBF to clarify their intentions. For now, the unmodified max HP is returned. The detected mod comes from: MedPod Verse.Log:Error(String, Boolean) EBF.EliteBionicsFrameworkMain:LogError(String, Boolean) EBF.Patches.Prefix_BodyPart_GetMaxHealth:PreFix(BodyPartDef, Single, Pawn) Verse.BodyPartDef:DMD<DMD?1658217088::GetMaxHealth_Patch2>(BodyPartDef, Pawn) MedPod.Building_BedMedPod:DiagnosePatient(Pawn) MedPod.Building_BedMedPod:Tick() Verse.TickList:DMD<DMD?822119936::Tick_Patch2>(TickList) Verse.TickManager:DMD<DMD?1578607616::DoSingleTick_Patch1>(TickManager) Verse.TickManager:DMD<DMD?968780288::TickManagerUpdate_Patch4>(TickManager) Verse.Game:DMD<DMD?-565676800::UpdatePlay_Patch2>(Game) Verse.Root_Play:Update()

sumghai commented 3 years ago

I'll take a look at this some time after the 1.2 update.

sumghai commented 3 years ago

Hi @Vectorial1024, as per the preceding comments, I've been informed that my MedPod mod conflicts with your Elite Bionics Framework, because I'm using the vanilla RimWorld GetMaxHealth() method in the following places:

https://github.com/sumghai/MedPod/blob/21f2757ebf548a487f8829e66ae2e89d0e612c82/Source/Building_BedMedPod.cs#L371

https://github.com/sumghai/MedPod/blob/21f2757ebf548a487f8829e66ae2e89d0e612c82/Source/Building_BedMedPod.cs#L394

Since I usually design my mods to use vanilla methods where possible, I'm not familiar with coding for compatibility with other assemblies, so I'm not quite sure what I need to do to have my mod play nice with yours (while still remaining usable for non-EBF players).

Any tips or advice?

Vectorial1024 commented 3 years ago

Ah hi there!

It previously has never been clear what exactly the "EBF protocol" means because I never quite tidied the docs after I paid my attention elsewhere, but eventually I have a rough idea how to describe it.

Roughly speaking, the problem that EBF has to face is the inability for it to distinguish between these two cases:

Because of how the data structure is set up in vanilla RW (e.g. BodyPartDef "templates" do not store BodyPartRecord instances), EBF requires extra params to be sent in during function call to determine which of the above two cases is intended.

That is what the "clarification required" means, do modders want the first case or the second case?

The main idea to establish compatibility:

From your side, you only need to make two new functions (or one, depending on your intentions) like this (or make it an extension method for convenience, it is still the same when observed from EBF's side):

// class must be public, otherwise EBF cant see it
// EBF had previously encountered another mod which set their classes to be internal
// and because it was an internal class, EBF couldnt do anything about it, even though the error was known
public class FooClass
{
    public static float GetMaxHealth(BodyPartDef def, Pawn pawn)
    {
        // first case, general values for general calculation, e.g., baseline stats for everyone
        // this is basically an extra layer wrapped on the original method
        return def.GetMaxHealth(pawn);
    }

    public static float GetMaxHealth(BodyPartDef def, Pawn pawn, BodyPartRecord record)
    {
        // second case, specific values for specific calculation, e.g., if David survives the Lancer snipe or not
        // notice that this contains the same content as the first case method above, except it has an unused BodyPartRecord param
        // that BodyPartRecord param will be useful to EBF
        return def.GetMaxHealth(pawn);
    }
}

and afterwards, for the example class provided above, you call that function as if it is an endpoint like this:

// we use the example case provided by MedPods
// here I guess that MedPods wants to know the specific max HP so that, perhaps, it can estimate how long the pawn will reserve the MedPod
// the mod author should know which version they want
float health = FooClass.GetMaxHealth(currentHediff.Part.def, patient, currentHediff.Part);

You could make other functions for coding convenience, but the point is, eventually, all convenience functions should call that custom GetMaxHealth function/endpoint.

And then your work is done. Observable effect still the same for everyone on your side, but now, there exists a common endpoint FooClass.GetMaxHealth(...) that EBF can target using Harmony to effectively patch calls to the vanilla MaxHealth function, and the error message will be gone.

And that is how I recommend setting up compatibility with EBF.

sumghai commented 3 years ago

@Vectorial1024 - I haven't had much luck following your instructions, I'm afraid.

I started out by adding a new class, and I chose to only implement the second use case that was relevant to MedPod:

using Verse;

namespace MedPod
{
    // Wrapper class for compatibility with mods like Elite Bionics Framework

    public class CompatibilityWrapper
    {
        public static float GetMaxHealth(BodyPartDef def, Pawn pawn, BodyPartRecord record)
        {
            return def.GetMaxHealth(pawn);
        }
    }
}

I then modified my existing method calls from the unmodified vanilla method to the one inside the new wrapper:

float currentBodyPartMaxHealth = (currentHediff.Part != null) ? CompatibilityWrapper.GetMaxHealth(currentHediff.Part.def, patientPawn, currentHediff.Part) : 1;
float currentHediffBodyPartMaxHealth = (currentHediff.Part != null) ? CompatibilityWrapper.GetMaxHealth(currentHediff.Part.def, PatientPawn, currentHediff.Part) : 1;

However, it seems that EBF is still not picking up that I'm now using the wrapped method.

What might I have missed?

Vectorial1024 commented 3 years ago

Ah, actually, my recommendation above required me to also make an update on my side, which I haven't done yet...

And, in the meantime, I re-read the C# docs, and remembered that we could use reflection to connect to EBF. This will mean that when you want to connect to EBF on your side, EBF never needs any code updates. This seems like a cleaner approach compared to my original idea of EBF manually targeting your code using Harmony. (The compatibility wrapper class is still recommended though.)

At this moment the docs and the C# endpoints at EBF are messy and not standardized yet, but there is a function at EBF that can serve as a makeshift endpoint for now. (Will standardize + tidy up later...)

We would do something like this in your CompatibilityWrapper.GetMaxHealth:

var makeshiftEbfEndpoint = Type.GetType("EBF.VanillaExtender")?.GetMethod("GetMaxHealth");
if (makeshiftEbfEndpoint != null)
{
    // EBF detected
    // effect same as writing return EBF.VanillaExtender.GetMaxHealth(def, pawn, record); but without the need to require a DLL
    return makeshiftEbfEndpoint.Invoke(null, new Object[] {def, pawn, record});
}
// EBF not detected, use vanilla
return def.GetMaxHealth(pawn);

Because Type.GetType("EBF.VanillaExtender")?.GetMethod("GetMaxHealth") can be quite heavy, you could optimize this by caching whether the makeshift endpoint exists during mod load. One way to do this is to do it like:

public class ModDetector
    {
        /// <summary>
        /// Detects if  EBF is loaded.
        /// </summary>
        public static bool EBFIsLoaded => LoadedModManager.RunningMods.Any((ModContentPack pack) => pack.Name.Contains("Elite Bionics Framework"));
    }

Or, perhaps even better:

public class EndpointsFooClass
    {
/*
TODO code:
function:
    if never cached:
        cache endpoint
        set already cached
    return cachedEndpoint
*/
        public static var MakeshiftEBFEndpoint => Type.GetType("EBF.VanillaExtender")?.GetMethod("GetMaxHealth");
    }
var makeshiftEndPt = EndpointsFooClass.MakeshiftEBFEndpoint;
if (makeshiftEndPt != null) { /* ... */ } /* ... */

And that is a better recommendation on connecting to EBF.

Vectorial1024 commented 3 years ago

Update: should still be EBF.VanillaExtender

sumghai commented 3 years ago

Ah, actually, my recommendation above required me to also make an update on my side, which I haven't done yet...

Whoops!

And, in the meantime, I re-read the C# docs, and remembered that we could use reflection to connect to EBF. This will mean that when you want to connect to EBF on your side, EBF never needs any code updates. This seems like a cleaner approach compared to my original idea of EBF manually targeting your code using Harmony. (The compatibility wrapper class is still recommended though.)

Yes, this sounds like a better approach.

I had something crude working, but the guys from Discord said I should be using delegates instead of Invoke, so I'm currently trying to fix some new bugs I've introduced.

sumghai commented 3 years ago

This should be good to go for the next update.