Error in Core.DLL Function for setting PID to match a Nature (Gen 3/4)

[evinjaff]

Describe the bug A clear and concise description of what the bug is.

While working on building an application that used the PKHeX.Core DLL, I found myself unable to properly set the nature on a Soul Silver savefile. More specifically, when I'd try and set a new PID (since natures in Gen 3/4 are determined by PID) it would set it to a random nature.

I confirmed that I was using the newest version of the DLL. I was calling the SetPIDNature function which seems to be the function with the error.

I was able to fix this in my own code by just looping the generation and checking the nature this way:

            Console.WriteLine((Nature)(mew.PID % 25) == pokemon.Nature);
            for (; !( (Nature)(mew.PID % 25) == pokemon.Nature ); )
            {
                // Attempt to set the nature
                mew.SetPIDNature(pokemon.Nature);
                Console.WriteLine("Nature after setting");
                Console.WriteLine(mew.Nature);

            }

To Reproduce Steps to reproduce the behavior:

1. In a C# script, link the PKHeX.Core DLL and include a Soul Silver save (called something blank.sav)

        internal void MakeShiny(PK4 pokemon) { 
            // Get IDs
            ushort trainer_id = pokemon.TID16;
            ushort secret_id = pokemon.SID16;
            uint pid = ShinyUtil.GetShinyPID(trainer_id, secret_id, pokemon.PID, 0);

            pokemon.PID = pid;  

        }

private static PK4 createDefaultEgg(uint tid, uint sid) {
    return new PK4
    {
        TrainerTID7 = tid,
        TrainerSID7 = sid,
        IsEgg = true,
        MetLevel = 1,
        Ball = 4,
        Species = 151,
        Nickname = "Egg",
        Language = 1,
        OriginalTrainerName = "PKHeX",
        OriginalTrainerGender = 0,
        HeldItem = 0,
        Ability = 8,
        Nature = 0,
        Move1 = 0,
        Move1_PP = 0,
        Move2 = 0,
        Move2_PP = 0,
        Move3 = 0,
        Move3_PP = 0,
        Move4 = 0,
        Move4_PP = 0,
        IVs = new int[] { 31, 31, 31, 31, 31, 31 }
    };

}

public void addEgg( EggCreator pokemon, int boxIndex) {

    SaveFile? currentSave = SaveUtil.GetVariantSAV("blank.sav");
    bool isShiny = false;

    Nature expectedNature = Nature.Timid;

    // hardcoded to gen 4 pokemon
    PK4 mew = createDefaultEgg(currentSave.TrainerTID7, currentSave.TrainerSID7);

    // Set the trainer ID according to the save file
    mew.TrainerTID7 = currentSave.TrainerTID7;
    mew.TrainerSID7 = currentSave.TrainerSID7;

    // Moveset
    mew.Move1 = 2;
    mew.Move1_PP = 30;

    mew.IVs = pokemon.IV;

    // Attempt to set the nature
    Console.WriteLine(pokemon.Nature);
    mew.SetPIDNature(expectedNature);
    Console.WriteLine(pokemon.Nature);

    // Set shiny after nature is set
    if (isShiny)
    {
        Console.WriteLine("Making Shiny");
        MakeShiny(mew);
    }

    box[4] = mew;
    // update box
    this.currentSave.BoxData = box;

    byte[] modifiedSaveData = this.currentSave.Write();

    // dump to location
   File.WriteAllBytes("testPROG.sav", modifiedSaveData);

}

Inspect the save file in either an emulator or PKHeX and see that the nature is not consistent

Expected behavior A clear and concise description of what you expected to happen.

The nature should match the Nature enum provided in the arrgument to SetPIDNature

Screenshots If applicable, add screenshots to help explain your problem.

Illustration for the code snippet in my own app showing that I need to run SetPIDNature multiple times to get a correct PID.

Additional context

I will link a pull request using the check from my app. It looks like maybe checking by enum over checking by byte might be a fix.

[evinjaff]

I took a closer look and will hold off submitting a pull request, because I'm not actually sure if the setting PID issue is necessarily a bug in the core, more of a lacking feature. It seems like the main disconnect here is that the SetPIDNature function called in a PK4 object is not aware it's being called on a Gen 4 game and as such will generate an incorrect PID. Theoretically game version should be set to fix this, but I think either the GameVersion does not contain a default value high enough to be considered gen 4.

With this context I see 3 possible solutions to help define this edge case:

1) Client code should just call GetRandomPID and not use SetPIDNature. Ideally client code should be able to use GetRandomPID though. Mark this somewhere in the function docs.

2) Give PK4 and PK3 overridden versions of SetPIDNature that always pass through a GameVersion value that's high enough to trigger the gen 3/4 check.

3) Set a default GameVersion value that is high enough to trigger a check when SetPIDNature is called

[kwsch]

Need to set a version to the Pokemon prior to calling the method. If it doesn't originate from Gen3/4 then it doesn't force the %25 correlation.

It's suggested that you use the Encounter Generator to prefill information rather than calling methods manually.

[evinjaff]

Ah ok, makes sense. Wasn't sure if there should be a guard implemented there, but that makes sense that it expects the version to be filled.

Also, is there a site for documentation on the Core DLL functions? I feel like as I'm writing stuff I'm kind of just poking in the dark for functions and touching things I shouldn't. The only help or information I've found on what functions do come from a few forum posts and the internal function documentation.

[kwsch]

Wasn't sure if there should be a guard implemented there, but that makes sense that it expects the version to be filled.

Yeah, it is expecting the object to have all properties available so it can determine which correlations it needs to follow. Sure extra guard clauses can be added, but since it's mostly an internal-use method it doesn't bother with them. The method in question is one of the very-old methods from when the program was originally written, hence the kludge.

Also, is there a site for documentation on the Core DLL functions?

Nope, that's why I tried to write as much xmldoc for the functions that are "useful". Since there are over 1000 classes/etc defined in the program it's a little hard to stay on top of documentation otherwise, and the code essentially acts as an code-example. There's still quite a few nuances/gotchas, as you've noticed :)