stenellad commented 4 years ago

47 #78

Looking at the code:
if (!IsPrisonerRecruitable(characterAtIndex) || characterAtIndex.Tier > 6)
{
      continue;
}
So Tier 6 shouldn't be recruitable, that's fine but the DefaultPrisonerRecruitmentCalculationModel returns the following:
  public override float[] GetDailyRecruitedPrisoners(MobileParty mainParty)
  {
      return new float[5]
      {
          1f,
          0.5f,
          0.3f,
          0.2f,
          0.1f
      };
  }
Which is the 'probability' that each tier gets recruited.

However Tier 0 isn't a thing (Well it is, applies to Heroes actually) for troops. So it's an off by one error, just need to replace that list with something like:
  public override float[] GetDailyRecruitedPrisoners(MobileParty mainParty)
  {
      return new float[5]
      {
          1f,
                        1f,
          0.5f,
          0.3f,
          0.2f,
          0.1f
      };
  }
for the mod, (Proper fix is of course just decrement the Tier by 1 in the actual code or use a different structure like a Dictionary to determine this, also the method name is a bit confusing...)

Can someone please explain to me how this conclusion was reached, and passed through your entire team without a single person noticing that the check is for prisoners greater than 6?

Edit: Also, the idea that "tier 0 doesn't exist" is extremely interesting to me, considering

public static int GetCharacterTier(CharacterObject character) { if (character.IsHero) { return 0; } return Math.Min(Math.Max(MathF.Ceiling(((float)character.Level - 5f) / 5f), 0), 7); }

Tyler-IN commented 4 years ago

Can someone please explain to me how this conclusion was reached, and passed through your entire team without a single person noticing that the check is for prisoners greater than 6?

You mean TaleWorlds?

stenellad commented 4 years ago

No, I mean you.

So Tier 6 shouldn't be recruitable, that's fine but the DefaultPrisonerRecruitmentCalculationModel returns the following:

This implies the person read "if (!IsPrisonerRecruitable(characterAtIndex) || characterAtIndex.Tier > 6)" to mean "All recruitable prisoners with tiers less than 6". This is clearly not what the line means. Through a pull request and the comments of at least 3 other people, nobody noticed this?

Tyler-IN commented 4 years ago

There's also a typo in the edited code block float[5] instead of float[6]

The check in game code is;

if (this.IsPrisonerRecruitable(characterAtIndex) && characterAtIndex.Tier <= 6)

I read this as 'If the prisoner is recruitable and his tier is less than or equal six'...

The character tier calcuation is;

return Math.Min(Math.Max(MathF.Ceiling(((float)character.Level - 5f) / 5f), 0), 7);

@tynakuh comments?

Tyler-IN commented 4 years ago

As far as passing through "our entire team" goes, the code looked fine and it was tested by tynakuh.

Is your mod different? It looks like you reached the same conclusion? https://www.nexusmods.com/mountandblade2bannerlord/mods/112

stenellad commented 4 years ago

My mod is indeed different.

Tier 6 units are recruitable without modifying anything but the underfull array. This is possible because the check that @tynakuh incorrectly read to mean "below 6" actually means "below 7".

If tier 6 units were not meant to be recruitable, the check would indeed be the way @tynakuh presumed it was, or the array values corresponding to tier 5 and tier 6 units would simply be 0 instead of empty.

Sorry for coming in with heat, but people are wasting my time by linking me every contrary opinion on what is obviously an unintended bug, and it concerns tier 5 and tier 6 units, not just tier 5.

Also, reading stuff like "tier 0 doesn't exist" because he didn't bother to read through how tiers are even determined makes me feel extremely alone, despite being in the company of people who are, like me, putting in a great deal of effort to understand and try to fix the relatively broken mess we have to work with.

Tyler-IN commented 4 years ago

So you propose the fix should be an array of; { 1, 0.7, 0.5, 0.4, 0.3, 0.2, 0.1 } (or at least 7 instead of 6) While we have (due to only prefixing the array with a 1); { 1, 1, 0.5, 0.3, 0.2, 0.1 } ( https://github.com/Tyler-IN/MnB2-Bannerlord-CommunityPatch/pull/78/files#diff-f12b6379fd7fe530de4b0d11f2589c3fR48-R53 )

Thanks for the bug report.

Feel free to see if any of our other patches have bugs.

stenellad commented 4 years ago

My creative liberty was the .7 and .4, you can change it however you want.

Technically it would be more true to the original design (maybe?) to use .1 for the tier 5 and 6 units. I'm not into these kinds of discussions, personally. My mod lets users put whatever they want.

My only issue was with the statement "Tier 6 units are not intended to be recruited by intended design", which is false.

Tyler-IN commented 4 years ago

So we should post-fix the array with two .1s, or something less.

stenellad commented 4 years ago

If you take a strictly "We're fixing bugs as close to the original game as we have information to", then probably, yeah.

I felt really dumb after I set the .7 and .4, because technically what I did was change tier 1 units to .7 instead of .5, and tier 3 units to .4, i.e. double the base game.

However, I also realized there's a bug that is slightly more difficult to fix, which requires rewriting DailyTick(). From my mod description:

Recruitment of prisoners currently works this way: every day, once for every unique stack of prisoner (that is recruitable and tier 6 or below), check an array for a value for their corresponding tier. If that value is above a random value between 0 and 1, add one prisoner in the stack as recruitable, and subtract the corresponding array value by 1.

The default game is missing array values for tier 5 and 6 prisoners, so they are never recruitable. Also by default, you can only recruit one prisoner per tier per day.

Chance to recruit can be edited via values.txt. each line, in order, represents the individual tiers of units, with the first line being tier 0. As of this moment, setting any individual value to e.g. 2 will guarantee two recruitments of a specific tier, provided there are two stacks of that tier. 1.5 would guarantee one, then have a (roughly) 50% chance on the second stack. Values between 0 and 1 serve as a percentage chance to recruit up to one unit of a tier, with chances given for each unique unit in that tier (since you would get multiple rolls).`

What I envision conformity would be like would be technically pretty simple to do even right now, just make every prisoner into an object and assign them a conformity value. This might lead to optimization problems, but the point is, I HIGHLY doubt that the intended FINAL version of this game is going to have a hard limit of one unit per TIER per day. That's just... silly. And arbitrary.

There are numerous ways to do it, but the idea would probably be some kind of loop through the second (middle) part of this code.

private void DailyTick() { TroopRoster prisonRoster = MobileParty.MainParty.PrisonRoster; float[] dailyRecruitedPrisoners = Campaign.Current.Models.PrisonerRecruitmentCalculationModel.GetDailyRecruitedPrisoners(MobileParty.MainParty); int num = MBRandom.RandomInt(prisonRoster.Count); for (int i = 0; i < prisonRoster.Count; i++) { int index = (i + num) % prisonRoster.Count; CharacterObject characterAtIndex = prisonRoster.GetCharacterAtIndex(index); if (this.IsPrisonerRecruitable(characterAtIndex) && characterAtIndex.Tier <= 6) { int elementNumber = prisonRoster.GetElementNumber(index); int num2 = this.GetRecruitableNumberInternal(characterAtIndex);

            if (num2 < elementNumber)
            {
                int tier = characterAtIndex.Tier;
                if (tier < dailyRecruitedPrisoners.Length && dailyRecruitedPrisoners[tier] > 0f && MBRandom.RandomFloat < dailyRecruitedPrisoners[tier])
                {
                    num2++;
                    this.SetRecruitableNumberInternal(characterAtIndex, num2);
                    dailyRecruitedPrisoners[tier] -= 1f;
                }
            }

        }
    }
    this.RemoveUnused(prisonRoster);
}

tl;dr I gave up on even pretending to balance my bugfix because this current limit is silly, and the required effort to fix the entire problem correctly, especially with patching, when they should do it themselves sooner rather than later, seemed to preclude the worthiness of it.

So yes, to your question, but I personally didn't do it because it's kinda really slow when you do it that way.

tynakuh commented 4 years ago

Thanks for the detailed explaination. You're correct I misread it completely at: if (!IsPrisonerRecruitable(characterAtIndex) || characterAtIndex.Tier > 6) I'd lean towards reverting the fix itself and removing it. Clearly intention is that tier 5 and 6 are not recruitable with their current array.

I was aware Tier 0 existed, but I could not find a single troop that was actually Tier 0 as they're all a minimum of level 5. (minus Heroes), thus the assumption, the wording was too strong and could be misunderstood I apologise. The changes made sense when I misread the original condition as characterAtIndex.Tier > 5 but that's clearly mistaken

I agree in the end either waiting for TW to implement conformity or to do it ourselves is the way to go giving it more leeway. The current way prisoner recruitment is handled just doesn't lend itself very well to any sort of fun.

Going ahead to push a revert; following the assumption that Tier 5/6 aren't recruitable by design. There's no real way for us to know the answer here though.

(The float[5] error was just in the ticket description itself)

BUTR / MnB2-Bannerlord-CommunityPatch

Prisoner Recruitment #114

47 #78