Brainstorming: refactoring classes/inheritances

[StefanP.MUC]

Since there was no other opinion and I re-thought about it, I made Tile inherit from GameEntity, too.

[oln]

Tile being a GameEntity sounds logical to me, nice work.

[StefanP.MUC]

What purpose exactly do destroMesh() and deleteYourself() serve? Sounds and reads to me as if this is what a destructor should do. So instead of calling "obj->deleteYourself()" one should be able to do "delete obj;". Or do they have another task/purpose than just stealing the work from the destructor?

edit: Notice: I'm currently working on a new class "Building : GameEntity, ActiveEntity, Attackable Entity" that holds everything common for Rooms and Traps (and possible later buildable things).

[oln]

It's for deleting the objects from the creature AI threads I think.
The renderManager currently uses the object itself when deleting the scene node, so the object can't be deleted before that has happened. I agree that the way this is solved now is not ideal. The render manager is also used for destroying the object afterwards, which is something that doesn't really belong there (It should only handle render-related stuff). I think we should consider an event handler class to do this deletion (and other things) instead, maybe even some kind of deletion queue.

[StefanP.MUC]

Yes, that sounds reasonable.

But currently I struggle with other design problems. It has basically to do with the AttackableObject interface and when objects are cast to it. AttackableEntity has:

• pure virtual getLevel()
• pure virtual getCoveringTilesl()

Both are called (often in different places...) from objects that are casted by static_cast<AttackableEntity*>.

But with this we can't move the level and the coveredTiles variables into their appropriate places GameEntity and Building, because we then can't cast Creatures to AttackableObject for coveredTiles access anymore - but the current code structure needs this behaviour.

I think the level problem can be solved by moving it into GameEntity and casting to GameEntity (should be possible because everything is a GameEntity anyways).

But how to solve the coveringTiles? The problem here is mainly the Creature class. It returns a std::vector with only one element (the Tile on wich the Creature is standing). From the human logical point of view coveringTiles should only be a property of Building (because only Buildings need more than one tile). But then we can'T Creatures to AttackableEntity anymore. But the fighting code needs this behaviour.
From the mathematical logic we could say drop single Tile storage everywhere and do everything with the coveredTiles vector (-> theoretically allowing creatures that are more than one tile big) and putting it into GameEntity. This could probably lead to a performance problems if we use std::vector very often.

Any ideas on how to sort and refactor these properties into the classes without having to drop big chunks of currently working code (=losing features)?

[MCMic]

I did not understand everything, but it seems to me that you need to decide right away if you want to have creatures that are more that one tile big.
But maybe this could be done later with a subclass of Creature? Not sure it will easily be done through…

[StefanP.MUC]

No, allowing bigger creatures code-wise would not mean to actually have such creatures.
The problem is not on the Creature site alone, it's more on the AttackableObject side. AttackableObject requires methods that do not make sense for all types of its subclasses (in this case Creature). This causes unlogical code structures (vectors that have always and exactly only one element and are never used - why have a vector at all then and why have this member at all then?) and unneccessarily complex type hierachy diagrams.

Allowing theoretically big creatures code-wise would solve the refactoring problem, but would not solve the unlogical type hierarchy.

[oln]

Maybe we could make "AttackableEntity" inherit from GameEntity, and then make everything inherit from that, instead of using it as an "interface". I would assume everything that can be attackable also is a GameEntity.

[StefanP.MUC]

Then we end up having Diamond inheritance, because MovableGameEntity is also inheriting from GameEntity. I already looked if I can separate these two, but there are similar problems...

[oln]

Ah, right, that complicates the issue.

[oln]

With the way getVisibleForce is implemented currently, the class could be dropped and replaced with a flag in gameEntity stating if the entity is attackable or not I think.

[StefanP.MUC]

This sounds like a nice approach. I will have a look later today.

[StefanP.MUC]

Hmm, this would solve a lot of problems, but not the most critical one:
getCoveredTiles() should be part of the Building class (at least that's what I find most logical)
But even if we merge AttackableEntity with GameEntity, we can not move getCoveredTiles() into Building because a lot of Creature methods need it (and a Creature is obviously no building).

The technical problem that prevents from cleaning the classes up is that Creature already uses to much stuff from AttackableEntity that logically doesn't belong to AttackableEntity. So the logical part of the problem is on AttackableEntity, the technical part is on Creature.

I think to fully solve this problem the whole attacking code has to be rewritten (another way would be to merge GameEntity, Buidling, Attackable, Movable, but this destroys the purpose of the refactoring and the object oriented coding style by creating god objects: https://en.wikipedia.org/wiki/God_object).

[oln]

The function is used to check where the object that is to be attacked is, the method for doing so will vary depending on the object, so I think it makes sense that it's there. (Maybe the name could be changed to better reflect what it's meaning is though.)

Or is there a better way to check this?

[StefanP.MUC]

Ah, now something gets clear to me... I think I confused some stuff here. Must be because of the endless functions doing more or less the same... Creature has also getPosition() (inherited from MovableGameEntity, returning a Ogre::Vector3) and positionTile() returning a Tile and the getCoveredTiles() only returns a single-entry vector with positionTile().

So, looks to me that position (Vector3) and coveredTiles (vector<Tile*>) should be 1) the only ones and 2) they should be handled by GameEntity. I think this would also make it possibel to de-couple GameEntity from MovableGameEntity. MovableEntity will then only provide functions like moveSpeed, destination and such.

Do you think this would be the right approach?

[oln]

Yeah, it can be a bit confusing.

Your approach sounds good to me.

[StefanP.MUC]

Hmm, not sure anymore if it will work this way...

MovableGameEntity also needs (for example, there are more) getName() at some places, which is a property of GameEntity. faceToward() is also a bit unlogical: it works with walkDirection, but what if we only have a rotating object that can't walk (cannon for example)...

I'm reaching more and more to the conclusion that we need one solution of these three, even if we planned not to use them:

1. diamond inheritance (GameEntity is Parent of Tile, Attackable, Movable and Active, all other classes like Building, Creature, ... are grandchilds of GameEntity)
2. force planned refactoring and drop not-anymore-working features (probably the most time-consuming)
3. leave it as it is now (working but unlogical code that probably will get more unlogical)

If we don't find another possible way to get the refactoring done, I'd vote for number 1) diamond inheritance.

[svenskmand]

Diamond inheritance is generally considered very bad code practice, as you probably already know. Which is also why Java does not have multiple inhericance, but only the ability to implement inferfaces and inherit from only one class. Why can you not use interfaces instead?

[StefanP.MUC]

Yeah, I know. But when using interfaces we have to implement the same code several times. Every subclass then needs to implement it's own get/setName(), get/setPosition(), get/setLevel() ... functions, for example.
The whole refactoring process was started with the intention to avoid these kind of code duplication. If we now start to re-implement these functions we wouldn't have to start the refactoring in the first place...

[oln]

Yeah, code duplication is not ideal.

Should un-attackable units have levels? (if not, level can be stored in attackableEntity)
It would be logical to split movableGameEntity into animated- and movableentity, as some things can be animated, and some also turn, but not move. (Maybe even some tiles will be animated, so I don't really know if there are many entities that shouldn't have the possibility for animations.) Maybe we should also think about how many objects that will be movable, but not attackable (missiles? maybe some of them could be attackable even.) If there are very few, then I think it would be okay to make all movable objects attackable (and use a flag for the few that isn't), avoiding the problem. (But using a tad more memory, though diamond/virtual inheritance does also use extra memory) I'm not talking about putting all functionality in one class, but this makes it possible to have a "straight" inheritance up to the specific objects.

[StefanP.MUC]

Let's make a table of what object types we have and what properties they need, including possible use cases:

Creature
Attackable - Yes.
Movable - Yes.
Active - Yes, but currently has its own doTurn(), could be replaced/renamed by doUpkeep()

Trap
Attackable - Yes.
Movable - Probably. Ideas: Mimics. Something like the Stomps from Super Mario (stone blocks that move back and forth on a fixed way)
Active - Yes.

Room and RoomObject
Attackable - Yes.
Movable - No. Maybe RoomObject moving on the Room, but generally not, I believe.
Active - Yes.

Tile
Attackable - Probably. Digging/Mining a wall/gold could be handled like attacking.
Movable - No.
Active - Probably. Ideas: Lava tiles maybe shoot out a fireball. Tiles that have a Geyser on them. Mana sources.

Weapon
Attackable - Probably. Ideas: Creatures can be disarmed if the enemy attacks the weapon instead of the holder. Destroyable weapons.
Movable - Probably. Ideas: self-returning throwable spears or axes.
Active - Probably. Ideas: Guns or similar need to reload. Magical weapons that need to recover after using their special feature (e.g. magical wands).

MissileObject
Attackable - Probably. Ideas: Cancel/weaken a fireball with water. Fast creatures could redirect arrows by hitting them.
Movable - Yes.
Active - Yes.

Conclusions:

• Probably everything can be attacked. So merging GameEntity with AttackableEntity should be fine, flag "isAttackable" is provided for non-default cases.
• Probably everything can be active. So merging GameEntity with ActiveEntity should be fine, flag "isActive" is provided for non-default cases..
• Movable has the biggest uncertainty. Rooms, RoomObjects and Tiles are fully static, some other things probably, too. Merging this is not logical. But how to handle actions like "change position/move"? The property "position" is common to all objects, but the class MovableEntity would have no access to it then.

Maybe making GameEntity a big class handling and providing attacking, moving and activity and solving the individual property availability by flags (simple bools) is really the best solution to get a clear structure. This would also be the most easy way to get scripting done, I think.

This would lead to

{l Code}: {l Select All Code}

           GameEntity
               |
   --------------------------------------------
   |           |          |        |          |
Creature    Building    Weapon    Tile    MissileObject
               |
           --------
           |      |
          Room   Trap
//And somewhere RoomObject, either also a Building or directly a GameEntity


Any opinions on this?

[oln]

I think that sounds good, nice work. We could potenttially make a movableEntity sub-class, for building, and tile. (the moving part of a trap could be made a missileObject) As long as it is a sub-class it can access the position from GameEntity. (Though this class should only contain the "walk" queue and other things that is used solely for moving the object around, as opposed to setting an absolute position.)

[StefanP.MUC]

Then I'll start with merging Attackable and Active. This shouldn't be too hard.

[StefanP.MUC]

OK, Attackable and Active are merged with GameEntity. This will make future cleanups, refactorings and changes much easier.

I also generalized AttackableObjectType into ObjectType. This can now be used for whatever reasons.

I think next thing will be to generalize some of the pure virtuals. I think there's a lot code that can be generalized and moved into GameEntity, no need to re-implement it.

edit: I just saw that only Creature and Trap actually uses experience. Does this make sense? Can a non-Creature have experience? For Traps it is only raised, but never ever used for anything. Traps can maybe be upgraded by workers, but not evolve itself, IMHO.
So, if there's no good reason to keep getExperience() in GameEntity then I'd remove it from GameEntity and make it a property only of Creature. This would remove a lot of empty pure virtual "implementations".

edit2: Same thing for level: Does a level make sense for non-Creatures? It's currently only used for Creatures and I can't imagine any case where non-Creatures could have a level.

[oln]

I suppose it could be interesting to let rooms and possibly traps level up, but I think it's okay to keep the experience to creatures only for now.