MonsterMHit and MonsterTrapHit refactoring

[k-bar]

In this PR I have tried to refactor MonsterMHit function as a contrubution to Issue #2350.

While refactoring I noticed a lot of similiar code with MonsterTrapHit function. Both functions are called twice in CheckMissileCol function, and trap called twice from object.cpp. To get rid of duplicated code I used Template Method pattern.

Result: 2 functions MonsterMHit and MonsterTrapHit are now merged into TryHitMonster. The number of parameters from 7 and 6 parameters has been reduced to 4 with the possibility of further reduction (read below). The complexity of the function has been significantly reduced (from 120 + 75 to 26 lines) thanks to the function extraction. This makes it more legible and polite. This PR should not make any changes to the game logic or fix any bugs. (I hope).

Details:

With Template Method as a method to solve the code duplication problem, I had to use some polymorphism. I created 2 child classes, PlayerMissile and TrapMissile, for code that differs between them, and the rest of the code is part of Missile as its method.

In 2 additional places in objects.cpp in BreakBarrel and UpdateFlameTrapthere there are calls to MonsterTrapHit, without creating real missile. To unify this function calls with refactored code, I added dummy missiles without adding them to the Missiles array - just for calling method purpose. They should only live in scope of these methods. To make this possible I had to provide TrapMissile class accessible as part of header file missiles.h.

I found that some code were so closely related to monster, that I transferred them to monsters.cpp and made them as Monster methods. It simplified the code in terms of number of parameters, and aslo get rid of some duplicates (as in player.cpp 's PlrHitMonst ).

Further refactoring steps:

As you may guess from examining the code, this is not exactly how to implement Template Method pattern in perfect world. Actually the Missile should be abstract class with virtual methods declared as pure virtuals. But I couldn't do that, yet, because Missile is beeing initialized, not its child classes. Therefore, I had to create converting costructors that I use in CheckMissileCol (can't downcast). For this reason, I had to put 2 placeholder definitions for CalculateCTHAgainstMonster and HitMonster. Probably further refactoring this domain would make MonsterMissile as well, and later initialize each missile the missile as child of Missile in AddMissile. Consequently, no placeholder would be needed, as Missile may be abstract, as it would have some pure virtual methods.

A further reduction in the number of parameterswould be draw mindam and maxdam into missile structure. For now, missile struct has only single _midam, and damage is calculated in several places, often without even updating this member variable. I would do it maybe another time, but I am afraid that this would break savegames compatibility or something. Can someone confirm this? We could also get rid of "shift" here at least as a function parameter.

I really don't like what CheckMonsterHit function was, what it's called, how it's used, what it does and how, but I excluded it from the scope of this PR. It is used in several places. For sure extra caution has to be taken when dealing with it. Its name was so misleading that I changed it to more informative LiftGargoylesOrIgnoreMages. To make sure the game behaves exactly as vanilla (at least no changes in this PR to game logic), I left the calls to it in the same places, to ensure random number is generated when missile hit mage with goal different than normal. Normally I would check for that before calculating Chance To Hit (and split gargloyle logic from mages). Maybe another time.

I wish I could pass monster "as reference" to functions more times (mostly in StartKillOrHitMonster part), but there are so many places the index is needed, that I left this out of scope.

Additional Notes:

All the changes do not change anything in the mechanics of Diablo or Hellfire, but due to unification of code and getting rid of duplicates I decided as follows:

• traps now check if monster is immune to Holy Bolt (but there is no Holy Bolt trap).
• traps now checks for acid resistance (but there is no single acid element missile in entire game (acid spitters damage is magic type)

It makes more sense to me to leave this in code, and you can use it for mods as well.

Bottom line

This is my frist PR. I have no practical programming experience, especially in the field of C/C++. (Usually I read code only). I am still learing. I am open to critique. And I hope you help me to get this merged.

[AJenbo]

Nice work. I'm seeing a lot of good changes in this PR. I do have a couple of notes regarding the changes.

• While LiftGargoylesOrIgnoreMages() does maybe better describe what the function does, it feels like it's more describing the implementation rather then the intent of the function. Maybe something like AiReachToHit() would be a better name.

• So fare we have avoided the use of Virtual function in the game code base, I know for one thing they can cause a hit to performance and we do target some rather low end devices.

• To better facilitate reuse a function should do just one thing (i know, lots of the code does not follow this pattern). Functions with an Or in the name almost always breaks this rule. Maybe StartKillOrHitMonster() can still be ok, if it was rephrased as ApplyDamageToMonster(), or something similar.

Maybe it would have been good to break this PR up in to several commits to make it easier to look at individual changes.

A further reduction in the number of parameterswould be draw mindam and maxdam into missile structure. For now, missile struct has only single _midam, and damage is calculated in several places, often without even updating this member variable. I would do it maybe another time, but I am afraid that this would break savegames compatibility or something. Can someone confirm this? We could also get rid of "shift" here at least as a function parameter.

The save game is pretty explicit at this point, so it's usually pretty clear if a change breaks compatibility (the compile fails).

The _midam property is saved a loaded to the missile portion of the save game, but if you can reverse the calculation during save/load then it should be fine to make the proposed change.

P.s. It will probably take me a bit to fully review the PR, and I may merge individual parts of it. Would you be ok with me rebasing your PR as I do so (I will still keep you as the comitter)

[DakkJaniels]

@AJenbo any thoughts on #4145? I had made some changes to how resists are checked, which would conflict with what was done here (not a huge deal, conflicts would just need to be resolved).

[AJenbo]

@DakkJaniels you can just ping me in the actual PR :D I'll have a look at it tomorrow.

[k-bar]

Yeah, I am not happy with those names. I treat them as temporary (by means of future refactoring iterations) but still informative at the moment. As I know what function does, just by reading its name. But they are doing too many things now, and thus they probably should get extracted to more functions (as a clean code rule from Uncle Bob - I love that serie- that function should do one thing). Such approach is probably not so friendly with lump files each ~4k lines. I wish some more structure is possible. Like modules, folders. But I have no idea how to decomposite such files. I was thinking about many names for this one. Gargoyle part is maybe related with AI, but mages - it is actually "treat them as immune to missiles, while they move around invisible". This is so weird those 2 such different things are in single function. Gargoyle part is responsible for taking hit, terminating missile, but taking no other consequences (like no deal damage, no tag WhoHit etc.). And mages part is - pretend the succesful hit never happened, let missile flow as nothing was in the way. It's more like CheckAndProcessUnusualBehaviorForMissileColisionWithMonster ;) I would just split it.

I just noticed I ommited resist part in that StartKillOrHitMonster, and I had to readd it. Suddenly i scrapped moving this function to monster.cpp, and I left it in missile.cpp (2nd commit here), because it appeared that PlrHitMonster in player.cpp stagger golems, and missiles stagger only monsters with m > 3... It was like that in vanilla :| This looks still bad and is duplicated, but with such exceptions that I can't find solution for in a moment. IT needs another refactoring iteration.

Yes, feel free to rebase and do whatever you want with this PR. Or letting me know what's wrong, because I am very unexperienced and want to learn. But I would be very happy if even some part of it would be merged.

[AJenbo]

I wish some more structure is possible. Like modules, folders. But I have no idea how to decomposite such files.

The answer: Slowly. You can see it already happening with something being moved to the header as well as all the sub folders that have been added to the Source folder :)

Yes, feel free to rebase and do whatever you want with this PR. Or letting me know what's wrong, because I am very unexperienced and want to learn. But I would be very happy if even some part of it would be merged.

The two reasons that makes me want to split is it that virtual is a bit controversial so I would like to narrow it down to a minimum to better evaluate it. And it's a very complex function so splitting up the changes makes it easier to reason about.

[k-bar]

I found a way to use template method pattern using (wait for it) function template ;) It provides compile time polymorphism. It can be first step, and later, when controversy will be gone, it can be changed to run-time polymorphism, abstract classes and other things to enable more code modularity, loose coupling and higher cohesion etc. (just pasted some wise words here). Cons: TryHitMonster now has 5 parameters instead of 4. However I have some plans to do something with that.

before:

now:

I read about virtual functions controversy on StackOverflow, and it seems that people's concern is VTable, but in reality it may be a bit slower due to code not beeing in CPU cache usually.

[k-bar]

another note: Comment on Disolving of LiftGargoylesOrIgnoreMages aka CheckMonsterHit commit. LiftGargoylesOrIgnoreMages (previously: CheckMonsterHit) function was used in TryHitMonster (previously MonsterMHit and MonsterTrapHit), PlrHitMonster and MonsterAttackMonster. The latter one is actually used to attack golem by monsters. Because monsters don't attack each other in vanilla. (but may be in mods). To standarize this in Monster::IsPossibleToHit now in addition to checking that the golem is not retreating Illusion Weaver :P it is also checking that he is not a non-normal-goal mage, or his mtalkmsg is none or isn't he charging right at the moment. Gargoyle part is moved to Monster::TryLiftGargoyle. (EDIT: forgot about berserk in HF, but the change is good imo). So - no changes to original game mechanics. I think it was ok to unify this, so the mods could make the monsters fighting each other just a bit easier. To further achieve vanilla compatibility, mages part has been moved to Monster::IsPossibleToHit and pushes RNG like in vanilla.

[k-bar]

I removed virtuals, and questionable function names, what else could I do to make this merged? At function complexity subject - I will describe in few words first commit:

• MonsterMHit and MonsterTrapHit had much of code doing the same.
• I moved general logic to Missile::TryHitMonster (later removed it from missile because I was not allowed to use virtual functions).
• I extracted some functions to monster.cpp as they really belong there.
• logic spefic to Player and Trap were moved to their classes (PlayerMissile and TrapMissile) as members (CalculateCTHAgainstMonster and HitMonster).
• common code from those specific function was extracted to StartHitMonsterByMissile and StartKillMonsterByMissile
• twice trap was called from object.cpp - so I adjusted those calls to use changed function.
• minor PlrHitMonster refactor as found code is doing the same as already extracted functions.

Thanks to all that - functions are small, well named, code is more clean, easier to understand. Maybe I could split it in more commits, but when I was doing the refactoring, I saw it as one change, and commited it only as it was shaped as working change.

Later commits were only virtuals->template function, additional simple code cleans + fix mistakes I made or make code gcc compatible

few other questions:

• should I put PlayerMissile class in cpp file? - since it is not used outside.
• should I move constructors implementation to cpp?
• what else could I change to make it better?
• clang said member functions shall be lowercase - I changed it in few places is that good?
• Has your view of virtual reality changed yet?

So I am awaiting for answers to question about what else to do to merge. Next, in another PR I think I could try to refactor Plr2PlrMHit, PlayerMHit - as another long functions called from within CheckMissileCol. Doing that would probably also work well with current changes to find and get rid of duplicate code.

[AJenbo]

Let me know if you need any help with rebasing :)

[k-bar]

Let me know if you need any help with rebasing :)

I was thinking about doing 1 more split, or even do all thing in this split but in proper order. Like

1. Extract Player and Trap specific code and make MonsterMHit and MonsterTrapHit almost identical in 1st commit.
2. Merge + change calls in CheckMissileCol and objects.cpp. That would greatly improve reasoning about changes.

And I consider extracting some little things a bit differently. Also found couple of my mistakes here. And yes, rebasing is a bit uncomfortable with current commit shape.

I wish I could do just iterated rebase and squash most of this things here in this PR, but I don't know if it is possible to do such things when PR is already open and commits are already pushed.. But since there are some changes I plan. I will do that split probably.

I consider just doing that 2nd split probably a finishing one, and then if it will be the case - I will just close this PR without merge, and leave it like that for documentation purposes.

[AJenbo]

I wish I could do just iterated rebase and squash most of this things here in this PR, but I don't know if it is possible to do such things when PR is already open and commits are already pushed..

It is, you just force push to the same branch.

I consider just doing that 2nd split probably a finishing one, and then if it will be the case - I will just close this PR without merge, and leave it like that for documentation purposes.

Ok, I will leave it up to you.

[k-bar]

I created split 2. I was thinking it is possible to review it while draft, but seems it is not possible :) So I converted it as ready for review.

[AJenbo]

I was thinking it is possible to review it while draft, but seems it is not possible

It is, but usually you won't get as thorough a review as draft indicates it is subject to change.