R-Vaccari / bannerlord-banner-kings

BSD 3-Clause "New" or "Revised" License
71 stars 1.35k forks source link

Reason for EscortMerchantCaravanIssueBehavior.ConditionsHold(Hero) Crashes #34

Closed ApoxPaito closed 1 year ago

ApoxPaito commented 2 years ago

Sorry for not coming up with a fix this time, but I have managed to figure out why this crash happens. I don't know how they are generated in the first place, I've scoured the source code and couldn't really see anything wrong with the only existing notable creation patch, but sometimes there will be Merchant notables in Villages that causes the crash. When the game tries to check issueGiver.CurrentSettlement.Town.Security, the game will throw a NullReferenceException, since the Merchants are located in Villages and naturally their CurrentSettlement is a Village and hence Town == null. I had written a simple cheat fix to remove these Merchant notables as they are generated myself, but maybe a better fix would be removing them daily or better yet, figuring out why they are generated in the first place and fixing that.

Image below should be an example to it. 20220416225501_1

ApoxPaito commented 2 years ago

I'm actively trying to figure out why this happens, and as far as I can tell, it's not related to the SpawnNotablesIfNeeded patch. Heck, it could even be some other mod or even a wild game engine bug too. I'll keep you updated if I can figure out exactly why it happens and how I can solve it, I thought opening this issue just as a heads up would be good too.

R-Vaccari commented 2 years ago

I had implemented notable killing in public version. That still won't get rid of them. I also have no idea why they kept spawning. I believe they don't anymore, in new games. And yes I know the causes are null pointers in methods associated with issues - I'm thinking what will be the best solution.

ApoxPaito commented 2 years ago

I've just noticed this particular code block inside Guild.cs, do you think it could be the culprit?

public override Hero GenerateLeader()
        {
            CultureObject culture = base.Settlement.Culture;
            IEnumerable<CharacterObject> templates = from x in culture.NotableAndWandererTemplates 
                                                     where (x.Occupation == Occupation.Merchant || x.Occupation == Occupation.Artisan)
                                                     select x;
            if (templates.Count() > 0)
            {
                CharacterObject template = templates.GetRandomElementInefficiently();
                Settlement born = Settlement.All.GetRandomElementWithPredicate(x => x.Culture == culture);
                // The line above means that a random settlement, including any Castle and Village belonging to the culture will be chosen
                return HeroCreator.CreateSpecialHero(template, born);
            }

            return null;
        }

I wasn't able to enter the said code block during my debugging endeavours so far, but this could cause random Merchant notables not just spawning in Villages but Castles too, and I also had crashes relating to Merchant notables in Castles. Sorry for not attaching a pic with a Merchant notable in a castle, I can't really locate any since the Encyclopedia doesn't show up notables in castles.

P.S. Also good thing I haven't really opened up a pull request either way then, because I kinda figured out that you might be aware of the issue and had a fix in mind. Even I had come up with one, I dunno how accurate it would have been with or without me changing things that already exist

R-Vaccari commented 2 years ago

Nice, very perceptive. I totally forgot about Guilds, it's not a finished feature. I see my code is wrong here and what you described can totally happen. Though I'm not sure any guilds are initialized, but this makes sense.