StatBlock cleanup

[clintbellanger]

stefanbeller started working on separating one StatBlock into a base class with several implementations -- mostly to separate shared values and specific values.

There was an issue with transformed stats, and the fix isn't straightforward, so stefan issued a revert.

Let's discuss here how we might structure the class, and what we might do when handling transforms.

[stefanbeller]

So I had a plan at the second half of the text at https://github.com/clintbellanger/flare-engine/pull/382

The StatBlock is an abstract base class, so it cannot be instantiated directly, but you can only create a subclass (AvatarStatBlock, NPCStatBlock or EnemyStatBlock).

The Entity class needs to have a pointer to a StatBlock and an abstract function statBlock(). (The Entity class is already abstract, so it's no big deal) The deriving subclasses would need to implement the statBlock function and return the specific stat block depending on its own type, i.e. the Avatar class will return an AvatarStatBlock, the NPC class will return an NPCStatBlock when calling that function.

The problem with the transforming of the Avatar could be solved similar as it is now: There will be 2 pointers charmed_stats and hero_stats which will carry two independent StatBlocks of different type (Avatar and Enemy). The actual stats pointer of the Entity class would just need to point to one of both. This yields the advantage to not have to copy over so much data as we're doing now. Now we have 3 StatBlocks (charmed_stats and hero_stats and stats), but operate only on stats. During transforming, one of the shadow copies will be taken and some of its data are copied over to the stats block.

If done right with the pointers, we would not need to copy anything, because of addressing always the right data anyway. (hero_cooldowns, slots, etc could stick to the Avatar block all the time, but movement, speed, resistances and such would be loaded from either of both blocks depending on the transform state.)

However we need to think about whether we'd like to change the StatBlock class at all. It works the way it is now. When I started rewriting it, I noticed we'd definitely need pointered objects instead of having them directly within another object, i.e. an Entity just had a StatBlock in place.

The new rewriting approach would require to have an indirection (pointer) to that StatBlock, hence make it slower, because dereferencing pointers and such more cpu than just accessing a desired member value.

On the other hand we would need save some memory. So let's assume there are about 100 entities on the map (let's pretend all of them are enemies) and we could maybe save 20 to 40 variables per StatBlock. This would result in a memory saving of 40 (saved variables) * 100 (number of enemies) * 4 byte (per variable) == 16000 bytes.

So we'd save less than 16kB of memory with this optimization, but would require more cpu cycles for dereferencing and finding the right class. I have no estimate nor benchmark how much more cpu cycles we'd need with that kind of rewrite.

So another thing to discuss would be readability and maintainability. So when doing such a rewrite, we'd end up with lots of classes, abstract classes and virtual functions, - the wet dream of an object oriented programmer. This can be more readable, iff you're used to the concepts and such. But as of now we had the mantra: OOP solutions as a last resort and so grew the team?

So maybe just leave it as it is now?

[dorkster]

So we'd save less than 16kB of memory with this optimization, but would require more cpu cycles for dereferencing and finding the right class. I have no estimate nor benchmark how much more cpu cycles we'd need with that kind of rewrite.

I think in our current state, CPU cycles are more valuable than having marginally lower memory usage.

But as of now we had the mantra: OOP solutions as a last resort and so grew the team?

Personally, one of the things that attracted me to the project was in fact the limited use of OOP techniques.

[meow-sokovykh]

Oh, I was thinking about initiating discussion like this and starting to implement separated stats. The StatBlock class now is total mess in my opinion. As I can see, there is need to introduce more and more stats, while removing hardcoded parts of engine, and introducing new features (e.g. recent fleeing and threating update made two new stats: #1470). Having 1k lines just for declaring variables would be... ehm, revolting?

Are u still agree, that we need that, or am I doing hard unnecessary necroposting here?

If the idea is alive, is the main requirement to separation is not to make much OOPish stuff? If so, I would like to try solving that somehow (no complete plan yet, need closer look into code).

[dorkster]

Let me start off by saying that this is not something we need to be too concerned with at the moment. The current implementation works. That being said, here is my current opinion on the matter.

I would like to move towards having the lines between Avatar, Enemy, and even NPC properties removed. Every entity would play by the same rules and make use of all the StatBlock variables. Think of it this way: we could take the player's control away and give it to an enemy/npc AI routine. Or do the opposite and give the player control of an enemy/npc.

I don't see how the code size of the StatBlock makes it any easier or harder to add/remove game-play variables. Splitting into sub-classes only adds another layer of abstraction that developers need to think about. It's actually more confusing now since there is stuff in the Avatar and Enemy classes that could be considered part of a StatBlock, but isn't for whatever reason.

[meow-sokovykh]

Ok so. If it's unneeded work, I won't do it. But I still think that StatBlock has lack of readability and maintainability in current state.