Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NPC/Mob/Vendor refactor #203

Open
CPunch opened this issue Mar 12, 2021 · 3 comments
Open

NPC/Mob/Vendor refactor #203

CPunch opened this issue Mar 12, 2021 · 3 comments
Assignees

Comments

@CPunch
Copy link
Member

CPunch commented Mar 12, 2021

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.

@dongresource dongresource changed the title NPC/Vendor refactor NPC/Mob/Vendor refactor Mar 12, 2021
@dongresource
Copy link
Member

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 and MobManager::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 CNSockets, which don't themselves hold data such as their in-world coordinates, angle, instance, etc. This complicates uniform handling. Perhaps we could introduce a PlayerEntity class that participates in the Entity 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.

@yungcomputerchair yungcomputerchair self-assigned this Oct 16, 2022
@yungcomputerchair
Copy link
Member

How much of this did the refactor address? i.e. what's left to clean up moving forward?

@dongresource
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants