devilutionX icon indicating copy to clipboard operation
devilutionX copied to clipboard

Object oriented Monsters

Open mikolaj-pirog opened this issue 3 years ago • 11 comments

Begin changing monster hierarchy to be OO. Those are very much first steps - as such no ground breaking changes have been made. This is done to experiment with OO monster design. I believe fully OO monsters would be easier to maintain and to understand.

I make this PR as a draft this early, since I would like to know if this even makes any kind of sense - there is much work to be done and I wouldn't like to do it without being sure it is even wanted in the first place. The code proposed is very very WIP and as such it does not make a lot of sense.

Here is a quick list of changes in the code:

  • Changed Monsters array to hold ptr to monster instead of monster
  • Added default ctor to monster
  • Added ctor to Monster that is a copy of InitMonster with initialization of fields that Init function did not touch
  • Remove adding monster through InitMonsters, monsters are constructed via Monster ctor and added to Monsters array. Changed monster adding accordingly. As of now, when new monster is added, memory for it is allocated via new, and the memory for old monster in the Monsters array is deleted.

I am sure there are some bugs - my goal was to quickly hack some code to spark a discussion about OO monsters: mostly if the idea makes any sense.

mikolaj-pirog avatar Jul 11 '22 16:07 mikolaj-pirog

This kind of memory management (allocating each monster via new/delete) is a lot less efficient than the current approach. We have DevilutionX ported to some very constrained platforms that can barely fit in RAM and barely manage 20 FPS as it is.

If we do decide to change how such arrays (Monsters, items, objects, etc) are done the implementation should still be efficient under the hood. For previous discussion, see https://github.com/diasurgical/devilutionX/issues/4742

glebm avatar Jul 11 '22 22:07 glebm

Per suggestion to make more efficient monsters handling I came up with using placement new. The memory for monsters is allocated only once via new. Then, everytime a new monster is being added to the Monsters array it is placed there via placement new, skipping memory allocation. If my understanding and implementation are correct, then this should be comparably efficient to master.

mikolaj-pirog avatar Jul 12 '22 15:07 mikolaj-pirog

An array of pointers means an extra indirection. If you're using placement new, you shouldn't need pointers.

The diff is also far too large to review because of all the . to -> changes.

See https://github.com/diasurgical/devilutionX/blob/master/Source/utils/static_vector.hpp for an example of how to implement a direct array via std::aligned_storage.

At this point, it's hard to tell whether this particular approach is the one we want, so please don't go through too much effort on this.

glebm avatar Jul 13 '22 08:07 glebm

The idea behind using array of pointers for Monsters is to later add each monster as a class inheriting base Monster and store them as a pointers to a base class

mikolaj-pirog avatar Jul 13 '22 10:07 mikolaj-pirog

Ah, inheritance, virtual functions, etc. Inheritance is usually not a good fit for complex software, especially games.

glebm avatar Jul 13 '22 16:07 glebm

maybe something can be done using composition?

AJenbo avatar Jul 13 '22 23:07 AJenbo

Ah, inheritance, virtual functions, etc. Inheritance is usually not a good fit for complex software, especially games.

Could you elaborate on that? It seems to me that managing monsters translates well into OO hierarchy.

mikolaj-pirog avatar Jul 14 '22 21:07 mikolaj-pirog

maybe something can be done using composition?

I thought about putting ai stuff into a separate struct. That would be mostly cosmetic, but would improve readability (it would be clear what monsters uses for ai). I also thought about maybe storing the ai function inside this struct, but it would increase the size of the struct.

What other things regarding composition do you have on mind?

mikolaj-pirog avatar Jul 14 '22 21:07 mikolaj-pirog

Not much it was a loos thought :)

One ugly aspect is that in town towners are seen as monsters but must be looked up in a different table.

AJenbo avatar Jul 15 '22 07:07 AJenbo

I thought about putting ai stuff into a separate struct. That would be mostly cosmetic, but would improve readability (it would be clear what monsters uses for ai).

How will putting the AI stuff in a separate struct be more clear than having it in the same struct?

I also thought about maybe storing the ai function inside this struct, but it would increase the size of the struct.

Let's not increase the struct size unnecessarily.

glebm avatar Jul 15 '22 10:07 glebm

Maybe something like:

monster.ai.goal;
monster.ai.var1;
monster.ai.var2;
monster.ai.var3;

It won't help with size, but could help structure things better?

AJenbo avatar Jul 15 '22 11:07 AJenbo

Polymorphism is not that good when thinking about performance. It is a good approach in theory but when we do have limited process power it will cause more issues than benefits. Another thing is that some devices won't handle allocating memory outside the heap well. So using new will be very slow.

fwannmacher avatar Aug 14 '22 22:08 fwannmacher