Fix unwanted npc removal

[thokkat]

Regression introduced in b7f8ed.

Problem was npcs with invalid waypoints are removed in Npc::resetPositionToTA.

Vanilla checks the current and next routine and would spawn npc only if one of these has a valid waypoint. But npc would still appear sometime later if there's a valid routine and is then never removed from the world. This needs more investigation and is subject for a future PR.

This would fix the Garwig case https://github.com/Try/OpenGothic/issues/798. Haven't tested others so far.

[Try]

Hi,

Have I understood, right, that it's not meant to be "the fix", but rather temporary workaround? Not spawning npc's is important to avoid stray monsters at (0,0,0).

At the moment, there are 3 set's of npc's:

    std::vector<std::unique_ptr<Npc>>  npcArr;
    std::vector<std::unique_ptr<Npc>>  npcInvalid; // dead or invalid TA
    std::vector<std::unique_ptr<Npc>>  npcRemoved; // removed, but may have a dangling references in game

However npcInvalid and npcRemoved are practically the same. Those are kept in memory, until level change or reload and never considered by game logic. It seem correct solution would be to

• move dead npc's into npcRemoved, and use npcInvalid for npc's with incorrect routine.
• allow resetPositionToTA/npc_exchangeroutine, for npcInvalid, and move them to npcArr if new routine is valid.
  • serialize npcInvalid into save file
• not sure about npc's with invalid spawn-point - probably send them to npcRemoved straightaway.

I'm unsure how access, from script, to npcInvalid should be organized. Things such as find/detect and etc.

[thokkat]

that it's not meant to be "the fix", but rather temporary workaround?

Basically yes. Garwig is removed because I removed the waypoint check in currentRoutine.

FUNC VOID Rtn_Start_608 ()
{	
	TA_Guard_Hammer	(08,00,23,00,"NW_MONASTERY_SANCTUM_02");
    TA_Guard_Hammer	(23,00,08,00,"NW_MONASTERY_SNACTUM_02"); // <-- SNACTUM
};

So I restored waypoint check to have alternative routine with valid waypoint taken. This would be the state before https://github.com/Try/OpenGothic/commit/b7f8edef7d2735b248df17a7244d4adb52df8f41.

New is that before returning static routine take the current. Waypoint is null for both so it doesn't matter but current would still allow insert helpers. They need ZS_Talk to work. This was the intention for removing the waypoint check but obviously incorrect.

allow resetPositionToTA/npc_exchangeroutine, for npcInvalid, and move them to npcArr if new routine is valid

My best guess is it has something to do with npc unloading if out of player range. Insert if in player range but since waypoint is not valid it's never inserted.

What I tested is if current routine waypoint is invalid on game start the waypoint from next routine is taken instead . If that is still invalid npc doesn't show up. I think the map guy in old camp has a routine like this but is still there from the start.

not sure about npc's with invalid spawn-point - probably send them to npcRemoved straightaway

You mean wld_insert point here? We had Nek in G1 where it was an empty string but npc was spawned at routine point. Can you tell me what this waypoint even does if npc is located at routine point anyway?

[Try]

Hi, Sorry - only today catching up on this review.

You mean wld_insert point here? We had Nek in G1 where it was an empty string but npc was spawned at routine point. Can you tell me what this waypoint even does if npc is located at routine point anyway?

Not 100% sure, but I wound assume it at least denotes position for monsters (or npc's without routines). If it breaks Nek, from G1, we can put those only to npcInvalid.

Basically, my assumption is:

• in vanilla, any way-point may happen during runtime
• if player approaches waypoint, that belong to particular npc - it get's loaded into the world.
• if way-point is invalid - player always considered far away, and npc have no chance to spawn (unless wp or routine will be overridden by script)

My suggestion is to put those npc's into dedicated array, and never have them represented physically in the game world, until Npc_ExchangeRoutine(or similar) called. Something that worry's me here - in theory, script can allocate too many npc's over time, causing form of memory leak. Memory leak, that persists across save-games...

[thokkat]

I wound assume it at least denotes position for monsters (or npc's without routines).

I gave this a test run and it's like you said. Empty routine means spawn at wld_insert point. Otherwise routine takes over, even if invalid and npc might not be spawned.

my assumption is:

I think this is how it's done except that the game is checking the previous routine waypoint too and spawns there if current is invalid. Changed the previous waypoint only for better visibility.

I'd say we do the same but check all previous waypoints and use then a valid one for spawning. This would avoid memory leak headache and possible npc pop ups in player's view in case routine is changed later.

Npc where all routines are invalid at spawn time are removed. There could be a later Npc_ExchangeRoutine with valid routines (or valid routines first and now invalid) but I'd argue (or hope) this would never happen because it would have been noticed during development if some npc was missing.

Not 100% accurate but would work for all possible cases I'm aware of.

[Try]

Generally sound good; one small adjustment:

Npc where all routines are invalid at spawn time are removed.

Since script can edit routines at any point by calling ta_min function, my suggestion:

• do not remove npc completely from memory, but store them in npcInvalid instead
• run garbage collection pass on those npc's from time-to time: if Npc::hnpc->use_count()==1, then there are no more references from inside Daedalus - we can safely move this npc into npcRemoved

[thokkat]

What about dead/wld_removed npc, should they be kept as well? They are just deleted now but can have use_count() >1.

[Try]

What about dead/wld_removed npc, should they be kept as well?

Now? We don't have wld_removed right now, or you mean with current proposal?

My idea was: if npc is dead, and resetPositionToTA reached, this npc will be moved to npcInvalid list. npcInvalid should be serializable, and persist after save-load.

if during GC pass, some npc in npcInvalid have use_count()==1 - those are moved into npcRemoved list. npcRemoved won;t be saved, and be cleared after save/load (similar to how npcInvalid works today)

In principle we don't have to have npcRemoved, if we can carefully cleanup non-countable references in C++, similar to how it's done for items, but we don't have to.

[thokkat]

I meant script function wld_removenpc . I thought double npc in "dead/wld_removenpc npc" sounds weird and only wanted to skip the "npc" ending but added a "d" instead which made it sound like an npc array and lead to confusion :D.

My understanding was this is only about npcs with invalid routine and if npcs which are currently put to npcRemoved should be treated the same. You answered that already with yes :).

[Try]

Obsolete by https://github.com/Try/OpenGothic/pull/845