search

IwoRosiak / DreamersDreams

GNU General Public License v2.0
2 stars 1 forks source link

Crash with Pawns that have no childhood (Warhammer-"ish" Dryad mod) #63

Closed slippycheeze closed 2 years ago

slippycheeze commented 2 years ago

This triggers on one of my Pawns, a Warhammer-"ish" Dryad who has no childhood, only an adulthood. Very sad or something, I'm sure, except they are horrors that spawn directly in their adult form and all... ;)

Anyway, point is, in this code:

https://github.com/IwoRosiak/DreamersDreams/blob/e2f97fa7e33445ba9a8bd0af46f3d1179190af47/1.2/Source/DreamersDream/DreamersDream/Dreams/Requirements/DreamRequirementsChecker.cs#L22

You carefully use a null-safe operator to ensure that pawn.story.GetBackstory(BackstorySlot.Childhood)?.spawnCategories doesn't fail when the childhood slot is empty, which is great... except.

The "except" is that C# turns that into null, and then tries to get an enumerator for a null collection, and blows up. boo. Generated IL code from that foreach loop:

   // [20 46 - 20 111]
    IL_0004: ldarg.1      // pawn
    IL_0005: ldfld        class ['Assembly-CSharp']RimWorld.Pawn_StoryTracker ['Assembly-CSharp']Verse.Pawn::story
    IL_000a: ldc.i4.0
    IL_000b: callvirt     instance class ['Assembly-CSharp']RimWorld.Backstory ['Assembly-CSharp']RimWorld.Pawn_StoryTracker::GetBackstory(valuetype ['Assembly-CSharp']RimWorld.BackstorySlot)
    IL_0010: dup

    IL_0011: brtrue.s     IL_0017
    IL_0013: pop
    IL_0014: ldnull
    IL_0015: br.s         IL_001c
    IL_0017: ldfld        class [mscorlib]System.Collections.Generic.List`1<string> ['Assembly-CSharp']RimWorld.Backstory::spawnCategories
    IL_001c: callvirt     instance valuetype [mscorlib]System.Collections.Generic.List`1/Enumerator<!0/*string*/> class [mscorlib]System.Collections.Generic.List`1<string>::GetEnumerator()
    IL_0021: stloc.1      // V_1

There at IL_0014 you can see it handle the ?. operator nicely, and put null on the stack instead of blowing up, then jump right to IL_001c and use it as an object. :(

Sadly, you have to either use null-coalescing with an empty collection (...GetBackstore(...)?.spawnCategories ?? new IList<string>{}), wrap the whole thing in a conditional (if (()?.spawnCategories is IList<string> match) { ... } or so), or use something like ()?.spawnCategories?.ForEach(cat => ...) from linq.

Anyway, fun times and all that, and thanks for the mod! I quite like it.

IwoRosiak commented 2 years ago

Hi! I am so sorry for taking so long.

Thank you for your most detailed explanation. I made that code a long time ago, and I was probably just null checking most random things that popped up in the code :) this code is long overdue for a small refactor, haha

I am currently working on my degree (only a few days left now), and as soon as I am done, I will be able to fix that bug. I can see you are investigating the code for 1.2, though. I have not updated the version for RimWorld 1.2 for a long time. I have to check if that bug is also present in 1.3; maybe I already fixed it :P

IwoRosiak commented 2 years ago

I fixed it but I have no way to test it right now :/ Would you be able to let me know if it works for you?