NPC/Mob/Vendor refactor
CPunch opened this issue · 3 comments
Moving the discussion over the NPC refactor to here. The problem with our current implementation of NPCs and Mobs is that it has become a sort of mix and match of different approaches, which has led to a pretty messy code base. Some of the things that were suggested for the NPC refactor so far are:
- Moving things into an abstract superclass (ie, a base Entity class which holds stuff like world coordinates.)
- A single class with a type enum for selecting behavior.
Any different approaches are welcome to be shared in this issue.
Concrete issues with the current design:
-
There are two constructors for
BaseNPC
. Both are unruly with the number of arguments they take; and they're semantically different in that one was originally meant to be used to construct summoned mobs which need to be deallocated when killed, while the other is meant for permanent mobs created at startup.The number of arguments they take has grown such that they cannot be distinguished without very carefully reading the number and types of the arguments supplied, which is very dangerous from a memory management perspective. Further, some invocations don't fit cleanly into either mold, static or dynamic.
These should be replaced with a more explicit system, such as a pair of descriptively-named static methods that call a private constructor. These should take fewer arguments; they can be configured upon construction and finalized by a separate function call that adds them to the world.
-
Eggs and sliders do not currently fit well into the system. Eggs are treated as a type of NPC, while sliders are sort of their own thing. Escort mission NPCs also wouldn't fit into the current system well, nor would custom, scriptable NPCs.
-
Mobs all currently use the same AI. The few specialized mobs (mostly Weeper/Injustice Foe on their own branches) are awkwardly special-cased. Non-trivial, non-static NPCs could have an arbitrary
step()
function or function pointer, which would allow for uniform handling of AI for all types of NPCs. -
Mobs are currently kept track of in both
NPCManager::NPCs
andMobManager::Mobs
. This redundancy is unnecessary and complicates construction/destruction. The performance benefit of iterating through a smaller map (as opposed to ex. skipping static NPCs when advancing mob AI) is presumably slight, as these are already very efficient data structures. -
ChunkManager is currently very redundant with how it handles NPCs and Players separately. Ideally, there should be a top-level
Entity
superclass that chunking operates in terms of; and all the details of what happens when one comes into view/leaves the view/is copied into a private instance, etc. should be handled by the object's virtual methods or some similar technique.The primary concerns with unifying players into this system is that we've been keeping the Player struct an aggregate/POD/C-compatible type, and would need to carefully examine if it's alright to make it into a non-aggregate class with virtual methods, etc. and what changes doing so would require.
In addition, we currently reference players by the pointers to their
CNSocket
s, which don't themselves hold data such as their in-world coordinates, angle, instance, etc. This complicates uniform handling. Perhaps we could introduce aPlayerEntity
class that participates in theEntity
hierarchy and caches data relevant to chunking? But this would likely be unruly and could complicate the issue even more. We must carefully consider how to best solve this problem. -
Nano powers and Mob powers are currently extremely redundant, and would benefit immensely from a uniform way to refer to NPCs and players. That way a single
doLeech()
function, for instance, could handle both players leeching mobs and mobs leeching players.
Note that everything that summons an NPC needs to first determine whether it's a mob or not, and then construct either a BaseNPC or a Mob object. There is a helper function called NPCManager::summonNPC()
that currently handles this; this is a good approach. Perhaps this function should be moved closer to the other NPC-related functions/methods that we end up writing.
How much of this did the refactor address? i.e. what's left to clean up moving forward?
Pending a more comprehensive review; we know that more changes are still needed to the AI state machine to properly support mob-vs-mob combat, which is a prerequisite for escort missions. There were certainly other things; I'll have to look into it again.