[All 1.20.X] finalizeSpawn() is always passed null SpawnGroupData

[sekelsta]

Minecraft Version: 1.20.1

NeoForge Version: 1.20.1-47.1.105

Steps to Reproduce: Base game effects - Create a new world and look for plains/savannah to find a horse herd. Seed 3040816188384926459 has one right near spawn. Observe that the group has mismatched color and markings, whereas in vanilla would have all been the same base color with different markings.

Modding effects - Create a mob that spawns in groups and attempt to add custom finalizeSpawn() functionality. Observe that the SpawnGroupData passed in is always null, even though it should only be null for the first member of the group and after that should be the data returned by the previous call.

Description of issue: When a mob's finalizeSpawn() is called, the SpawnGroupData passed in is always null, causing vanilla horse herds to spawn with mismatched colors and patterns and limiting what mods can do.

[embeddedt]

@Shadows-of-Fire Can you look at this one? I noticed this when looking at the patches but thought it might have been intended.

[Shadows-of-Fire]

Hm? It shouldn't be, and isn't as far as I can tell. The event firing site is here https://github.com/neoforged/NeoForge/blob/e7ac32353de099d69c6ac137d8cb836f97b0863f/src/main/java/net/minecraftforge/event/ForgeEventFactory.java#L284-L295

The vanilla call is directly translated to that hook using the method redirector coremod, so the parameter shouldn't be lost there either.

[sekelsta]

Also occurs on neoforged versions 20.2.88, 20.4.234, and 20.6.48-beta Let me know if an example mod that logs the SpawnGroupData passed in would be helpful

[sekelsta]

Even though the code does look like it shouldn't be affecting anything, I tried deleting the finalizeSpawn method redirection from https://github.com/neoforged/NeoForge/blob/1.20.1/src/main/resources/coremods/method_redirector.js (thank you for linking that), and indeed it does change the behavior back. Here is the herd of horses near spawn on world seed 3040816188384926459, generated with 8f8559b:

And here is the same herd of horses on the same seed after I deleted the method redirection, matching the vanilla world:

Maybe the core mod redirection is somehow not preserving the return value? I'm not super familiar with the system, but I am very very excited to get this fixed, so if there's anything I can do to help, please let me know.

[sekelsta]

Found it. onFinalizeSpawn() is just completely ignoring the return value of mob.finalizeSpawn() and returning event.getSpawnData(), which cannot be updated in the normal way and so will always be null unless set from an event subscriber.

If I change the method to:

@Nullable
@SuppressWarnings("deprecation") // Call to deprecated Mob#finalizeSpawn is expected.
public static SpawnGroupData onFinalizeSpawn(Mob mob, ServerLevelAccessor level, DifficultyInstance difficulty, MobSpawnType spawnType, @Nullable SpawnGroupData spawnData) {
    var event = new MobSpawnEvent.FinalizeSpawn(mob, level, mob.getX(), mob.getY(), mob.getZ(), difficulty, spawnType, spawnData, null);
    boolean cancel = NeoForge.EVENT_BUS.post(event).isCanceled();
    SpawnGroupData spawnDataOut = null;
    if (!cancel) {
        spawnDataOut = mob.finalizeSpawn(level, event.getDifficulty(), event.getSpawnType(), event.getSpawnData());
    }

    return spawnDataOut;
}

then horse spawns match vanilla again. Should I run all the tests and make a pull request for this?

[Shadows-of-Fire]

I'll roll a fix for it into #880