Creature rework (checkpoint)

[swwu]

Just to save a bit on merging pains down the road, and to get wider playtesting on the current state of the rework. Most things should be up to feature parity with pre-rework, with some small exceptions:

• grabs/grabbing techniques and disarms don't work in melee.

What's been done so far:

• Creature base class for all things (that move) ever. Character base class for the player, human NPCs, and other such dialogue-enabled folk.
• Unified stat/bonus system. All bonuses are calculated once per turn in Creature::reset_stats (instead of on-the-fly everywhere), and Creature::get_<stat> will automatically include the bonus in the total value. Can also get base in get_<stat>_base or just the bonus in get_<stat>_bonus. Not fully implemented as a lot of bonuses still occur in e.g. player::suffer, but these shouldn't affect anything except the accuracy of the get_<stat>_base functions. "Stats" include, but are not limited to, str/int/per/dex, speed, bonus armor, bonus damage, damage multipliers, stealthiness, resistance to throwing techniques, etc.
• Unified combat mechanics (everything but throwing weapons for the player, everything not from monattack for monsters). To copy what I said in #4970:

The idea right now is for all attacks and attack-like things to go through the same code path. These functions are Creature::melee_attack + Creature::deal_melee_attack for melee, and Creature::projectile_attack + Creature::deal_projectile_attack for ranged. The attacking_creature.<x>_attack functions roll the attack/generate the damage/etc from attacking_creature against a specified target, and target_creature.deal_<x>_attack deals an attack with a specified damage and hitroll to target_creature, which can then dodge/mitigate the attack.

All attacks go through one of these, e.g. gunfire goes through player::fire_gun, which generates the bullet damage/dispersion values from the player's stats, and then calls player::projectile_attack (which then computes the trajectory and calls Creature::deal_projectile_attack on whatever gets hit, etc).

• Player diseases and monster effects now unified under Creature effects. About 1/3 of the diseases and all of the creature effects have been converted to the new effects system.
• More fleshed out damage types and resistances. Not all types and resistances are fully implemented (so, e.g. freeze grenades don't do cold damage yet, they just do regular grenade damage, and acid resist doesn't properly protect against acid) but a good deal are in. Borrowing from #2030, the damage types are:

    DT_TRUE, // typeless damage, should always go through
    DT_BASH, // bash damage
    DT_CUT, // cut damage
    DT_ACID, // corrosive damage, e.g. acid
    DT_STAB, // stabbing/piercing damage
    DT_HEAT, // e.g. fire, plasma, lasers
    DT_COLD, // e.g. CBM heatdrain, cryogrenades
    DT_ELECTRIC, // e.g. electrical discharge

• All the damage types can be mitigated separately, and monsters can resist specific types of them. Damage types now also "carry" special effects, so e.g. all cold/electric damage will reduce moves as well as deal damage, and all heat damage carries a chance of setting you on fire depending on magnitude. These are mostly selected to keep in line with the auxiliary effects from the "old" system (where every instance of electrocution was always accompanied by a moves penalty, etc), but are up for discussion should they seem distasteful to people.
• A lot of combat/damage numbers are now carried in floats, so resistances can work at a higher granularity.
• Since monsters now use the same melee combat mechanics as players instead of special monster combat mechanics, their skills are considered at a -2 penalty when making attack rolls (a fair number of relatively common zombies had melee skills >=8, namely dogs). This is a temporary fix until we fully integrate and balance monsters in the "new" system (wherein they can have real stats).

[swwu]

@kevingranade Less a concern about exposure, more a concern about getting this in before I leave for the holidays (the 20th) after which I won't have consistent access to internet for a few weeks. Exposure is a nice side effect though.

The BOUNCE flag, burstfire retargeting, and underwater misfire penalties are back in. Not sure if it's worth putting the grab/disarm stuff back in, as there aren't any techniques that actually use the grabs flag (all the "grab" techniques just use down_dur which is implemented), and disarms only really do anything against NPCs, which we're tossing out for the time being anyways.

[narc0tiq]

Uh, does this gender-specificity need to here? It's the same text.

[kevingranade]

Grab and disarm are [del]fairly[/del] very low priority, so that's fine. Burst-fire retargeting is fairly high priority, bounce and underwater penalties are solid nice-to-haves. It looks like you'll have it sorted out enough to merge within a few days, which would be nice, since I'm heading for a long workholidaycation... thing next Monday and will have spotty ability to merge things until the new year as well.

[swwu]

@kevingranade burst-fire retargeting, bounce, and underwater are all accounted for. I'll see what I can do about the style but it might be easier for someone who can actually run astyle to just run the indicated files through it. Anything else needs doing?

[kevingranade]

By the way, this is why I said to make a do-nothing creature class (then merge), then extract pieces of code from monster and player a bit at a time (merging as you go). Please keep this in mind in the future instead of trying to tackle the whole thing as a single branch. Not only is this more painful, but it's getting less review and testing than it would if we were proceeding incrementally, and it's not as bisectable if you do have some weird bug hiding somewhere.

[kevingranade]

This is a pretty strong nice-to-have, it's a major defense of a fairly common monster (shocker zombies).

[swwu]

This is in. Go check monster::deal_damage_handle_type. This is just a rogue comment.

[swwu]

Oh wait never mind, I misread the part of the file. Yeah, I can add this real quick.

[swwu]

@kevingranade That's more or less what was done, minus merging back into master. I've also been merging master into this branch the whole way so there's no reason it shouldn't be bisectable.

[kevingranade]

Definitely a nice to have.

[swwu]

This also currently breaks the encapsulation pretty badly, since monsters now use the same combat system as humans (which prioritizes the eyes/head/torso with higher attack rolls). Would need a fair bit of extra scaffolding to get in place.

[kevingranade]

I suspect that was the case, that's an acceptable temporary regression IMO.

[kevingranade]

This definitely needs to be fixed, otherwise it will turn misses into hits fairly frequently.

[swwu]

How? The targeted square is still offset the way it's supposed to be.

[kevingranade]

if we're not using it, delete it.

[kevingranade]

This is fairly critical functionality for some monsters.

[swwu]

Noted.

[kevingranade]

if bounce is done this should go away.

[swwu]

Noted.

[kevingranade]

Again this is critical, pretty sure burst fire is broken if you don't have this. You'll just keep shooting the same spot even though you're trying to shoot a new target.

[swwu]

No, it definitely works. I've tested it. I can attach a screenshot if you like.

[kevingranade]

Woah! you can't just change the whole accuracy system with no discussion.
Revert this and PR separately.

[swwu]

The numbers work out to be the same, I just changed where they were added. The 80 is selected, for instance, because that's what all the constants in the old numbers added up to be. The only difference is that the standard deviation will be slightly smaller in this because it uses a fixed number of rolls instead of rng-ing each separately. Unfortunately I can't PR this separately because the old system violates encapsulation pretty badly. All of the "mechanical" changes are as close as possible to the old system as I could manage, in the sense that their expected values always match up and their standard deviations are usually fairly close to matching up.

[kevingranade]

The results might be similar, but they aren't the same, and this version of the system is much less legible than the previous version, where it was extremely straightforward that each source of dispersion contributed a random amount between 0 and its max value. If you feel you need to pull all the constants and logic into this method, that's fine, but keep the logic the same.

[kevingranade]

Looks like that's all the pending issues, sort those out and it looks good to go, modulo testing.

Development methodology discussion below, has nothing to do with this PR any more:
@swwu "Less a concern about exposure, more a concern about getting this in"
It needs exposure, the means we have for doing that is to push to mainline and wait for complaints, which is why I want more frequent merges and more incremental development. If you'd gone out of your way to get more exposure and testing for the branch, I'd feel better about it, but as it is I have no confidence that this all actually works, and I can't possibly exercise all the changed functionality myself. If things do break, it will be a mess to find out what went wrong.
"That's more or less what was done, minus merging back into master."
It's precisely merging to master that needs to happen, along with the changes being small enough to review. If you PR smaller chunks, I can actually exercise each chunk reasonably before merging it, I have no possible way to test this other than the barest sanity. Also if people find things it's far easier to find and fix problems as we go instead of digging into the history of the branch.

The bigger your PR is, the more likely it is that I simply won't have time to deal with it. This one didn't reach the threshold, but that threshold isn't fixed, it changes based on how much time I have available, which is a steadily shrinking supply. I'm sorry if I'm coming across as harsh, but the way you developed this imposes a lot of extra work and uncertainty on me, and this is the only way I have of protecting myself from that.

[swwu]

@kevingranade Cata's codebase is a big pile of functionality without any central guiding principle, where every class member variable is public and you have to explicitly check hp <= 0 each time you do damage to a monster. You said you wanted to do "no-holds-barred refactoring". To me, refactoring is taking that pile of functionality and introducing structure to it. I want to make things like #4970 possible.

That kind of refactoring isn't possible with smaller chunks. I can't just change the melee attack functions to use creatures instead of monsters+players without dropping half the features attached to melee attacks, because so many of those features depend on very specific player/monster peculiarities. I can't just change the gunfire function to use creatures without dropping half the features attached to gunfire, because so many of those features depend on very specific player/monster peculiarities. If i were to commit code in small chunks that I hacked into working, we would end up barely better than where we started - we would have a "unified base class" in name only.

I appreciate that it takes you time to merge these, and I appreciate that you've done so. But I can't do a rewrite that has one-for-one all the features of the original and is done in small chunks and is comprised of code that isn't a steaming cesspit of spaghetti. I'm only human; the best I can do is "pick two".

Edit: Since I didn't actually make this clear, what I'm saying is, I can do small merge commits if you'd like, but they almost certainly either won't have immediate 100% feature parity, or will contain monstrous hacks.

[dwarfkoala]

Has this been taken to the forum for exposure yet?

[moist-zombie]

I don't comprehend 98% of this, but it looks quite a bit like HARD WORK :D Effort noted, to all involved parties :)

[kevingranade]

Ok, I THINK that addresses everything, so this will move to the top of the pile for merging for me. Thanks a lot for all this hard work. I hope I don't give the impression that it's not appreciated, this is a major undertaking that really needed to happen.

Semi-unrelated policy side discussion:
To clarify my position a bit too. I think this PR could have been done in about 3-4 PRs, possibly introducing then removing some hacks along the way and overall taking either more or less work*, but I'd be a lot more certain that it's fully reviewed, and it would have gotten a lot more testing. Even breaking it in half would have made a really big difference.

*More raw code, less time spent merging for all parties, honestly don't know where it would come out as far as total time spent.