How Indie Stone Fixed the Bandana 90% Protection Bug... Poorly

Edit 2021-10-27: Indie Stone released Project Zomboid build 41.56, and at first look I think they've fixed the bug correctly.  I'll write a post that goes into detail soon.

Two weeks ago, I posted about how, because of a bug, bandanas effectively gave you 90% protection from zombie attacks (from standing zombies attacking you from the front).

About a week after I posted about bandanas, Indie Stone posted a changelist for their upcoming (at the time) 41.55 Build, and they mentioned "Fixed bandanas providing far too much head protection"

Two days ago, they released 41.55. 

This bug was not fixed correctly - it was a band-aid.  If I'd seen this bug fix as a code review, I would have sent it back so they could get it right before release.

What Was the Bug?

I had actually filed a bug on the Indie Stone forums in August, but at the time I didn't realize the ramifications.  When I realized what was going on, I posted my sensational example, that bandanas were effectively giving 90% protection.

You can look at my posts about the bug, but here's the summary:

Sometimes, when a zombie successfully attacks you, a piece of clothing will fall off of you and nullify the attack.  These items of clothing are exclusively worn on the head, and are basically hats/helmets, glasses, and some masks.

The chance that a piece of clothing falls off during an attack is one of the attributes (ChanceToFall) of the clothing defined in the scripts.  Football helmets had a ChanceToFall of 3.  Bandanas had a ChanceToFall of 5.  Baseball caps had a ChanceToFall of 80.  Glasses had a ChanceToFall of 50.  The adjusted value was modified to be larger if the attack was directed at the head or neck.

This line from the code was the problem:

if (((Clothing)inventoryItem3).getChanceToFall() <= 0 || Rand.Next(100) < n) continue;

Where n is the adjusted chance to fall value, and the code that came after was the code that made the clothing fall.

It basically says if the random 1-100 number is less than the adjusted chance to fall, DON'T FALL.  It should  have been > n, not < n.  Easy mistake to make.

Note that it also says that if the clothing's defined ChanceToFall is 0, it will never fall, despite the flipped <.

So the result of this bug was that a bunch of pieces of clothing that should almost never fall off were falling off regularly (bandanas, motorcycle helmets, football helmets).  Also, a bunch of hats that should fall off most of the time weren't (tin foil hats, baseball caps, beanies).

How Did They Fix It?

1. They removed the feature of falling clothing protecting you from attacks

They went from 41.54

        if (Rand.Next(100) > n4) {
            n = 1;
            boolean bl4 = false;
            if (this.getParentChar().helmetFall(n3 == BodyPartType.ToIndex(BodyPartType.Neck) || n3 == BodyPartType.ToIndex(BodyPartType.Head))) {
                return false;
            }

To 41.55

        if (Rand.Next(100) > n4) {
            n = 1;
            boolean bl4 = false;
            this.getParentChar().helmetFall(n3 == BodyPartType.ToIndex(BodyPartType.Neck) || n3 == BodyPartType.ToIndex(BodyPartType.Head)); 

They basically removed that helmets falling would return false, meaning that "helmets" falling (a poorly named method, because it covered glasses and masks as well) no longer nullified successful attacks.

2. They adjusted the ChanceToFall values to 0 for SOME of the items, allowing those items to catch the other condition that I mentioned in the code snippet earlier (and so, never fall).

Changed items (plus a few more) with 41.54 values:

Military Helmet                       10
Bandana (Head)                         5
Kentucky Baseball Helmet              10
Riverside Rangers Baseball Helmet     10
Z Hurricanes Baseball Helmet          10
Crash Helmet                          10
Motorcycle Helmet                      3
Police Motorcycle Helmet              10
USA Crash Helmet                      10
Firefighter Helmet                    20
Football Helmet                        3
Hard Hat                              20
Mining Helmet                         20
Hockey Helmet                         10
Jockey Helmet - 1                     10
Riding Helmet                         10
Riot Helmet                            1
Airforce Helmet                       10
Spiffo Suit Head                      10
Bandana (Tied)                         5
Bandana (Face)                         5

Changed items (plus a few more) with updated 41.55 values:

Military Helmet                       10
Bandana (Head)                         5
Kentucky Baseball Helmet               0
Riverside Rangers Baseball Helmet      0
Z Hurricanes Baseball Helmet           0
Crash Helmet                          10
Motorcycle Helmet                      0
Police Motorcycle Helmet               0
USA Crash Helmet                       0
Firefighter Helmet                    20
Football Helmet                        0
Hard Hat                              20
Mining Helmet                         20
Hockey Helmet                         10
Jockey Helmet - 1                     10
Riding Helmet                         10
Riot Helmet                            0
Airforce Helmet                        0
Spiffo Suit Head                      10
Bandana (Tied)                         5
Bandana (Face)                         5

(I'm glad I wrote a little python script to extract these values while researching my other post!)

So, in 41.55, falling clothes no longer protect you, and certain (inconsistently determined) helmets aren't falling anymore.

But they didn't fix the actual bug I had mentioned.  Certain helmets weren't falling anymore, but others were - the code that was causing helmets to fall but tin foil hats to not usually fall was untouched.

My Reaction?

This is a half-assed solution.

I thought it was a cute feature that sometimes an attack could be nullified by a piece of clothing falling off.  Someone at Indie Stone did too, otherwise they never would have implemented it.  If they wanted to remove it that is certainly their decision.  But based on the other part of the fix I don't think they thought it through.

Changing the ChanceToFall values for SOME of the items shows that they didn't actually understand or investigate the bug they were fixing.  Motorcycle helmets were falling off too easily - so the solution is to change the ChanceToFall from 3 to 0?  They changed the ChanceToFall vales to 0 for a handful of items.  What about all of the other items?  Army helmets were also falling off too easily, but they were left at 10.  Firefighter helmets and Hard hats, which I think are the most common of these very protective helmets, are still falling off way more than they should.  They should actually fix the bug.

Edit: What Would I Do?

This section was written half a day after I first published this post, based on a comment cool_fox made on Reddit.

What would have been the result if they'd done my suggested fix, which was to swap the < for a > or >= in the snip of code I referenced earlier.

                int n = ((Clothing)inventoryItem3).getChanceToFall();
                if (bl) {
                    n += 40;
                }
                if (inventoryItem3.getType().equals(string)) {
                    n = 100;
                }
                if (((Clothing)inventoryItem3).getChanceToFall() <= 0 || Rand.Next(100) < n) continue;

This is something I think they should still do.  It will fix the problem where secure hats/helmets are falling more often than hats that are not well secured on the head.

But some hats and helmets would still be falling off more than they should.  Baseball caps would be falling off 100% of the time when the neck or head was hit, and 80% of the time otherwise.

To maintain the spirit of the preexisting code, I'd let clothes continue to fall, but ONLY when the head or neck was targeted.  I'd also let them continue protecting the wearer when clothes were knocked off (because I thought it was a cute detail).  I'd remove this code, because the purpose of it is to make items fall off more easily when the head or neck is hit.

And in the BodyDamage class I'd basically change this 41.54 code:

        if (Rand.Next(100) > n4) {
            n = 1;
            if (this.getParentChar().helmetFall(n3 == BodyPartType.ToIndex(BodyPartType.Neck) || n3 == BodyPartType.ToIndex(BodyPartType.Head))) {
                return false;
            }

To something like this:

        if (Rand.Next(100) > n4) {
            n = 1;
            if (n3 == BodyPartType.ToIndex(BodyPartType.Neck) || n3 == BodyPartType.ToIndex(BodyPartType.Head)) {
                if (this.getParentChar().helmetFall(true)) {
                    return false;
                }
            }

The intent would be to ONLY allow for helmets (and glasses and masks) to fall if the attack targeted the head or neck.  At that point, baseball caps would fall off (and protect) you at 80%, and bandanas would fall off at 5%.  And I'd restore the old values of ChanceToFall that were changed from 41.54 to 41.55.  

I'd then re-evaluate the ChanceToFall values for items that should fall easily to make the fall a little less easily.  Should glasses fall off at 50%?  Should tinfoil hats fall off at 80%.  I'd first look at cutting all of the values (including the small ChanceToFall ones) in half.  I wouldn't want any item falling off and protecting you 50% of the time or more - that way, it would be a happy little accident rather than protection you could count on having.  Glasses would fall off and protect on 25% of head/neck hits, and baseball caps would fall off during only 40% of head/neck hits.  Reasonable enough.

And I'd look to see where else helmetFall was being called, because I wouldn't want to break the code elsewhere.

And then, since I'm not that familiar with Project Zomboid code (I'm not someone who looks at PZ's code professionally), and I don't own this piece of code, I'd get someone to take a look to confirm I'm seeing everything I think I should be seeing.  I'm sure if I owned the code, I'd be much more familiar with it and would have an easier time finding the consequences of the changes I'm proposing here.

I'd also look into refactoring helmetFall.  There's some duplicate code there, with half the code pertaining to zombies and the other half to players, but many of the lines are identical.  I suspect the reason this bug originated because a change was made to the zombie half of the code but wasn't copied to the human half of the code.  I can accept that at some companies, under some deadlines, you might accept a little sloppy duplication in your code, but if that code causes a bug because the code wasn't adjusted in all places, that is a strong argument to fix that duplication problem.

I'd also want to change the name of helmetFall, since the name doesn't quite match what it does.  helmetFall checks to see if certain headgear (hats/helmets, glasses, masks) falls, and then makes the headgear fall, and then returns true if it did fall.  If I wanted to continue keeping that as one method (for simplicity), I might change the name to "doesHeadgearFall".  I have a little grudge against the name "helmetFall", because the first time I examined this part of the code I assumed it only dealt with helmets/hats, and I initially thought it made the helmets fall instead of randomly determining if the helmets fell.  Now I have a distrust of method names in the rest of the Project Zomboid code.

Now, back to the rest of the original post.

Should They Fix Their Processes?

I don't know.  It depends.  How many bugs are they creating?  What is the cost of their bugs?  How much time are they spending fixing bugs rather than doing new development?

A Code Review probably would have helped.  A second set of eyes catches a lot of the things the first set misses.  But do they need code reviews for bugs like this?  Do they have the time?  It is a small team working on a game and they are almost desperately trying to get multiplayer out.  This is the kind of bug that hardly anybody notices, so the consequences of just giving it a quick shake and putting a band-aid on the most sensational part are small.  

Unit testing?  I like unit tests, but it probably wouldn't have helped here.  If you think the bug is that bandanas are giving too much protection and a handful of helmets are falling too easily, you are only going to write unit tests for bandana protection and checking if your motorcycle helmet stops falling.

But, if they missing things like this bug fix, what else are they missing?  

Epilogue

The Indie Stone isn't writing rocket guidance software or medical device software or even financial software, so the consequences of 99%+ of their bugs is very small.  And nobody expects their software to be 100% bug free.  But when they're fixing something they know they got wrong on their first pass (a bug), they should probably spend a little more time trying to do things right.  Because the people who discovered the bugs are watching closely.

Thanks for reading!  If you think I made a mistake or have any questions you'd like me to answer by looking through the code, let me know!