Revert "Create splash attacks and give them to Boomers (#71964)"

[Fris0uman]

This reverts commit c3e8b12.

Summary

None

Purpose of change

#71964 Touched a lot of code and was probably merged a bit too fast, upon review the core feature of the PR relies heavily on magic numbers and esoteric logic that will be extremly difficult to build upon and document in the future. I suggest to revert it early before it gets too entrenched.

Describe the solution

Describe alternatives you've considered

Testing

Compile and starts without errors
Start game before revert > get covered in bile > Save
Load save after revert > no issues

Additional context

I'm probably going to re implement the liquid attack stuff in guns since a lot of the infrastructure already exists with liquid ammo and spread calculation already being a thing, while in spells everything would have to be done from 0

Fixes #72160

[worm-girl]

Multiple devs reviewed the PR, which (combined with the larger one it was split off of) was open for a few weeks. As requested, I simplified the calculations involved, there are no magic numbers left that I can discern, unless you want to count the 0-200 saving throw penalty caused by having 0-100 breathability, but I'm not sure I'd agree.

So what is the next move? Does anyone have a better model to propose or specific feedback for how it ought to be done instead? I had apparent support from several people that made me feel pretty confident I was working in the right direction.

[kevingranade]

The scent change is fine, though the exclusions applied to it seem rather arbitrary. This is the part that's best aligned with the original intent of the boomer.

In broad strokes the infrastructure around the splash attacks seems reasonable, though the code to handle dodging seems rather BIG considering what it's doing, which I think is a big part of what Fris0uman is getting at.

The filthy clothes part is problematic, and as far as I can tell from the numbers is going to get triggered every time one of these attacks land. There's been a very very long history of people proposing that "X should cause filthyness" and consistent pushback that it's difficult to work it in without causing a repeated chore that players have to deal with, making things that cause it annoying rather than challenging. For a rare effect that can be dealt with rapidly it's a possibility, but this seems pretty common and does nothing to make it easier to deal with.

Similarly the volume of material required to make this happen is greatly exaggerated, a chemical attack that works by getting in your eyes or irritates your skin in even small amounts is one thing, but a volume great enough to heavily soil your clothes at a distance is a ridiculous amount of filth to be throwing around, and presumably it's something that's triggered repeatedly...

Yes the spitter zombie has some of the same problems and we should also look into changing or removing it.

Multiple devs reviewed the PR...

Sorry, this is just irrelevant, things don't get locked in just because they got reviewed or merged, even I get things removed when they don't work out as intended.

[Maleclypse]

If I’m understanding correctly the desire is for this to have a simpler implementation that allows it to be built upon more easily in the future. If that’s incorrect hopefully someone will correct me.

[RenechCDDA]

Multiple devs reviewed the PR, which (combined with the larger one it was split off of) was open for a few weeks. As requested, I simplified the calculations involved, there are no magic numbers left that I can discern, unless you want to count the 0-200 saving throw penalty caused by having 0-100 breathability, but I'm not sure I'd agree.

So what is the next move? Does anyone have a better model to propose or specific feedback for how it ought to be done instead? I had apparent support from several people that made me feel pretty confident I was working in the right direction.

Here are some rather straightforward issues, which I hope you can understand and find agreeable.

-Not one of the mentioned contributors approved your PR. Review is not approval. If someone wishes to review and approve your PR, they can specifically do so. Here is an example if you are unfamiliar.. Obviously, there is no strict requirement for approval, but I just want to make that clear. I reviewed your PR. I did not approve of it. I do not wish for you assume otherwise.

-There were many, many, many unresolved conversations on the first attempt (#71584). And...

-Because you opened another PR, those conversations vanished and could no longer block merging. And...

-Because your commits are... well, this(image)

And...

-Because your commit history remains unorganized, it is nearly impossible for someone to tell if concerns were addressed, why things were added, what the status of the PR is, or anything. It's ok if your commits are unorganized if you're making small, uncontroversial changes which can be easily reviewed. It is significantly less okay when those commits stretch across a dozen C++ files.

When you decided to close your existing PR and reopen it as another after I had already spent some effort reviewing it and I personally threw my hands in the air. When you do that, it means that anyone willing to take the time to review your code has to double their own work, because we cannot track your commit history (if we tried to do it that way we'd more than double our work...).

[worm-girl]

When you decided to close your existing PR and reopen it as another after I had already spent some effort reviewing it and I personally threw my hands in the air. When you do that, it means that anyone willing to take the time to review your code has to double their own work, because we cannot track your commit history (if we tried to do it that way we'd more than double our work...).

I was told by Kevin to split my PRs up. I didn't just arbitrarily decide to close it.

-Because your commits are... well, this(image)

What am I supposed to be doing instead? I learned to code by working on this project and have zero familiarity with github or workflow or anything else outside of my PR history here, so I'm genuinely asking. I thought the method was to commit as individual tasks were completed to make rollbacks easier.

The filthy clothes part is problematic, and as far as I can tell from the numbers is going to get triggered every time one of these attacks land.

That's not the case. The order of operations for applying filth is, roughly:

1. Boomer casts a dodgeable spell with an attack roll of 3
2. If the player fails to dodge, they are hit by a splash of bile, with a liquid volume of 42 units (according to the spell json)
3. Coverage is rolled on the body part, starting with the outermost layer, as it is with any physical attack
4. If the roll is under the coverage on the outermost item, the bile hits that item
5. The item makes a saving throw of d200 + (200 - breathability * 2) against the liquid volume of 42. If it fails this roll, it enters the "item is potentially affected" part of the function. This is where things like damage to the item, applying flags, or whatever else can be rolled for
6. At this point, if the spell has the filth-applying flag, the item makes a second identical saving throw of d200 + (200 - breathability * 2) < volume. There are two saves here for parity with the way items are damaged by attacks - they get multiple saving throws against the damage, so it makes sense that something similar would happen with filth. It is not appropriate as far as I can tell to copy the method used in resist_damage or absorb_damage, as they are working with different numbers to represent the item coming apart, not getting gore stuck to it.

As listed in the PR, the percentage chances for an item struck by the full volume of a bile attack from a basic boomer are:

Boomer vs 0 Breathability Item = 2.75% chance to become filthy
Boomer vs 50 Breathability Item = 4.84% chance to become filthy
Boomer vs 100 Breathability Item = 10.89% chance to become filthy

So there is a 2.75% to 10.89% chance for a single piece of the character's clothing to become filthy if they are hit with a bile barf attack, which most characters can easily dodge. In testing, a character with even nominal dodge was only hit when they slipped on bile and fell prone, or when they were dealing with multiple other enemies in melee, which is the intent - boomers aren't dangerous on their own, they're a force multiplier for others.

7. The splash attack then calculates how much of the fluid soaks through to be able to affect the next layer based on the breathability and coverage of the item it hit. In most logical cases (IE it hits a raincoat), no fluid gets to the next layer, so nothing underneath can be affected. The player gets pretty solid feedback in the form of descriptive strings about what is happening:

The case above is a huge boomer, which casts the spell at level 4 so has slightly higher odds to affect equipment. Note that the shirt did not become filthy in this image. I had to get barfed on about 10 times for the coat to become filthy.

Filthy gear is something that the player currently never really has to deal with, except in the very early game when they might be compelled to put on some filthy riot armor in order to escape a dangerous area and wash it or make something better. After that, it simply never comes up again. Filth is a mechanic that has very limited impact, only reducing morale and potentially causing wounds to become filthy if that body part is hit, something that's currently very easy to fix, so it doesn't seem particularly onerous that a player might occasionally have to think about throwing an item away, washing it, or just living with the filth effect.

Similarly the volume of material required to make this happen is greatly exaggerated,

This feels like a problem with boomers in general. They throw around an absolutely cartoonish amount of goo in L4D, amounts that their bodies could not possibly contain. If you're saying that that's fine and rather my math is wrong and the 42ml splash attack shouldn't be able to make a shirt filthy, I don't disagree. We could easily turn the volume up and the rate of filth per ml down, but @Fris0uman mentioned that there are already too many magic numbers. As far as I can tell, this breathability-based saving throw is the only one, and I can't think of an empirical way to represent this that is simpler than what I've already done, so I'd need some guidance there.

If I’m understanding correctly the desire is for this to have a simpler implementation that allows it to be built upon more easily in the future. If that’s incorrect hopefully someone will correct me.

If someone would like to take a shot at it, or has suggestions for ways it could be mathematically simplified that wouldn't make the filth rate way more common than it ought to be, I'm all ears. I'm not claiming to be much of a coder.

The ultimate intent behind this project is to fix acid, which I think is currently not implemented in a way that really reflects what having acid splashed around the place would be like. Zombies should not be playing "the floor is lava unless you are wearing shoes", they should be trying to melt your skin and your armor. I started with bile because getting bile on you is not that big of a deal comparatively, and once (if) it's implemented in a way people are happy with, it'll be fairly easy to work from there.

It may also be noted that theoretical attacks above a certain volume would have a 100% filth rate - no such attacks are currently possible, but the plan for that was to figure out some kind of logic for attacks that approach that volume to split and affect different body parts, representing a larger splash covering more area. This way, for example, a theoretical zombie black dragon with a bile breath attack instead of acid in magiclysm could hit you pretty much everywhere with its attack, but your items could still potentially save vs filth.

[anoobindisguise]

The biggest issue I see personally is that filth is all or nothing - either clothing is filthy or it isn't, and there's only one kind of filthiness (straight from a corpse). That makes it hard to apply gradually with a barf attack, but for an outright death explosion it seems pretty in line to me. If we're hard-against boomer bile causing filth on clothing, it's possible that breathability can influence how long scent lingers rather than how likely it is to become filthy. I personally think in case of a point blank death explosion filth seems reasonable (if boomers become more rare) and given that, the discrepancies pointed to as problematic can be resolved by tweaking numbers in JSON rather than outright removal.
It looks like the intent of the system is to generalize it to acid as well. If devs believe that its current implementation will make that too difficult that's a different matter then.

[Zireael07]

@anoobindisguise That's a preexisting issue with FILTHY, not with this PR, though

[Theawesomeboophis]

I think the boomer overhaul should be reworked, not reverted. Boomers were pretty basic walking landmines before and these updates have fleshed them out and made them more threatening.

[Fris0uman]

The revert is the first step, I'm also cooking some fixes and salvage of this on the side but it's going to take time.

[Theawesomeboophis]

So, this is just temporary and a proper rework is planned?

[worm-girl]

So, this is just temporary and a proper rework is planned?

l asked for help fixing the magic number problem, but nobody's responded, and the fact that this is being reverted entirely kinda takes it out of my hands.

[Kamayana]

What am I supposed to be doing instead? I learned to code by working on this project and have zero familiarity with github or workflow or anything else outside of my PR history here, so I'm genuinely asking. I thought the method was to commit as individual tasks were completed to make rollbacks easier.

Not a dev, so I can't speak for them, but I think it's best to make every commit focus on a specific feature/change/fix in its entirety, with a descriptive commit message explaining exactly what you're adding there. This can be accomplished by either holding off on committing until you've finished everything, or by squashing all the little commits together at the end and replacing the "update ____" messages with a more descriptive one of what they all accomplished as a whole.

For example, this is a good commit: 62d6213. It's laser targeted to a specific change, and described clearly. A reviewer looking over the commits can see "okay, this code was changed to make the explosion aoe smaller". There's the remaining question of why you decided to make the boomer's explosion smaller, but that can be handled in the PR description (if you really want to do an extra long commit comment, you can add two line breaks and then all text after that will show up in github as extra descriptive text that can be read by clicking the little "..." bubble. I went way too long without knowing that).

A less clear commit would be this one: e071b3d. It's a vague commit message and does a whole lot over a lot of different files. It adds effects, adds spells, changes existing effects, changes descriptions, adds multiple new c++ functions, and modifies existing c++ functions. It would be clearer if all of those things were staged and committed separately, with a message explaining what each one is changing. There was also a particular issue where when you split off from #71964, you added in all the borrowed code in one big lump a198c12. A better approach there would've been to cherry pick appropriate commits from the bigger branch to the split-off one, which is another advantage of keeping your commits targeted - so you can cherry pick specific features.

And the thing is, your PR descriptions are very well thought out and descriptive, so you're basically doing all this already. You can even just copy text from one to the other - I've done that.

Basically, my first big PR #64334 had way too many commits, and my eyes glaze over scrolling through that list. But I used what I've learned to make my most recent big PR #70692 way more focused and understandable. I actually like reading the latter's commit list - it's basically a clean timeline of how I constructed the PR.

[Inglonias]

Be that as it may, I have never heard "unclear commit messages" or "unclear commit history" used as justification to revert something in this or any other project before now, and I've been working on software professionally for over five years, longer as a hobby.

[RenechCDDA]

Be that as it may, I have never heard "unclear commit messages" or "unclear commit history" used as justification to revert something in this or any other project before now, and I've been working on software professionally for over five years, longer as a hobby.

To be clear, that is not why it is being reverted. That is an explanation for how it got merged. The commit history meant that nobody reviewed this stuff properly, nobody could review it properly.

It is being reverted because of the multitude of issues that Kevin and Fris0uman are pointing out.

[AnotherSeawhite]

What on Earth have I started

[kevingranade]

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/soiled-clothing/29534/10