devilutionX icon indicating copy to clipboard operation
devilutionX copied to clipboard

MonsterMHit and MonsterTrapHit Split 2

Open k-bar opened this issue 3 years ago • 5 comments

Please review this PR - as 2nd part of split of #4570.

This second part of MonsterMhit and MonsterTrapHit refactor I finally merged both function into TryHitMonster using template method pattern. To do so I used function template to still achieve proper inheritance. Player specific code went to PlayerMissile class and Trap specific code went to TrapMissile. Both classes are derived from abstract class Collidable, which I have plans for in the future :)

It is strongly recommended to view changes commit by commit, because there are many movings of code.

I am also looking forward for permission to merge already implemented solution based on abstract class and pure virtual functions. If not now, then maybe in the future.

This PR should not change game mechanics. Minor changes:

  • enabled damage calculations for Bone Spirit as viable trap missile.
  • fixed Hellfire bug related to Ring of Fire trap vs monsters.
  • firewall originating from Oily shrine calculates CTH the same as trap Ring of Fire.
outdated This PR will focus on 2 things

1st commit will extract Player and Trap specific code from MonsterMHit and MonsterTrapHit.

It won't be ready to merge yet!

After some review remarks I will commit 2nd commit in which I will merge those 2 almost identical right now functions.

Please note that number of parameters will drasticall change, and names will go shorter, as all function names starting with PlayerMissileSomething or TrapMissileSomething will be PlayerMissile::Something and TrapMissile::Something. So you do not have to focus on that right now.

k-bar avatar May 22 '22 13:05 k-bar

MonsterMHit and MonsterTrapHit merged now. Please suggest naming, placing in file and namespace and other remarks in review. This version of solution is based on template function pattern using template function. But In my opinion it can be converted to use virtuals too. If not now - then later. I also consider moving MissileHitMonsterConsequences into Collidable but then it won't be pure interface ;) but maybe it won't be - because of later Plr2PlrMHit and PlayerMHit similiar refactoring. After resolve all concerns it should resolve #2350 issue imho.

k-bar avatar May 23 '22 16:05 k-bar

For easier view of changes - look only at commit changes, because Files changed view mess this up.

k-bar avatar May 23 '22 20:05 k-bar

if you wonder how it would look with virtuals instead function template: https://github.com/k-bar/devilutionX/commit/cb92aec7ef0509f375514d063a0c0bf9246ed7d3 https://github.com/k-bar/devilutionX/commit/e075fc42f9f4ba024712f7ef1adbe27dd6dbcb8f

EDIT: constructor from 2nd commit actually should be merged here. It simplify again break barrel thing.

k-bar avatar May 24 '22 13:05 k-bar

not really, but i can tell you really like virtuals :)

AJenbo avatar May 24 '22 14:05 AJenbo

not really, but i can tell you really like virtuals :)

why not, go take a look it's prutty. But seriously I think it may be the way to go, if not now then later, when missiles go further refactoring. Btw, some low-end devices might go test it now of there is any noticable overhead.

k-bar avatar May 24 '22 16:05 k-bar