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 memory fixes and NPC VScript expansions #337

Conversation

Blixibon
Copy link
Member

@Blixibon Blixibon commented Jan 2, 2025

This pull request contains several fixes or enhancements for NPC enemy memory as well as a large number of new VScript hooks and functions variously related to enemy memory, sensing, and general scheduling.

There are a lot of closely related changes in this pull request which are difficult to clearly separate, but these are the highlights that can be itemized:

  • Several new VScript hooks and functions on CAI_BaseNPC for sensing, NPC memory, and general task/schedule behavior.
  • Fix for GetTaskID VScript function not returning correctly scoped ID (ai_basenpc.cpp:944)
  • MarkEnemyAsEluded can now broadcast to the entire squad if ai_squad_broadcast_elusion is set to 1.
  • TASK_GET_PATH_TO_ENEMY_LKP_LOS now correctly uses the enemy's last known position. An oversight in the code previously caused it to use the enemy's actual position instead, making NPCs who use the task accidentally cheat. This fix can be disabled with ai_enemy_memory_fixes if the fixed behavior is undesirable.
  • A new SensedObjectsManager VScript singleton gives the ability to add unique objects (e.g. physics props) to the sensed object list, allowing NPCs to see them in the same way they would see players and other NPCs.
  • The VScript functions for squad data now use integers instead of strings, as even though squad data supposedly accepts any parameter, only integers are capable of save/restore. Since these functions were very obscure (and only partially functional), this is unlikely to break existing scripts.
  • New VScript functions on CBaseCombatCharacter for glow effects. (addendum: this requires E:Z2 code and it wasn't as related as the other changes anyway, so I will include it in a separate PR)
  • New OnTakeDamage, ModifyOrAppendCriteria, and CanBeSeenBy VScript hooks for CBaseEntity. Also exposed RemoveContext, which was oddly missing.
  • Fix FireBullets VScript hook using incorrect parameters.
  • New FreeSound VScript function for CSoundEnt.

These changes were all made for the development of Progenitors, a currently unreleased mod with an emphasis on stealth mechanics that run almost entirely on VScript, although I was planning on committing these upstream from the start.

Due to the volume of changes and some uncertainty regarding save/restore compatibility, I would wait until v8.0 before merging this.


PR Checklist

  • My PR follows all guidelines in the CONTRIBUTING.md file
  • My PR targets a develop branch OR targets another branch with a specific goal in mind

@Blixibon Blixibon force-pushed the mapbase/feature/npc-memory-and-vscript-expansions branch from 6db2932 to 290d9fb Compare January 2, 2025 15:55
@Blixibon Blixibon marked this pull request as draft January 2, 2025 15:58
@Blixibon Blixibon force-pushed the mapbase/feature/npc-memory-and-vscript-expansions branch from 290d9fb to 85cc7f2 Compare January 2, 2025 17:05
@Blixibon Blixibon force-pushed the mapbase/feature/npc-memory-and-vscript-expansions branch from 85cc7f2 to 970887d Compare January 2, 2025 17:24
Copy link

@1upD 1upD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments of things I wanted to get clarification on. I couldn't find any issues that I can tell.

@@ -740,6 +744,25 @@ void CAI_Squad::UpdateEnemyMemory( CAI_BaseNPC *pUpdater, CBaseEntity *pEnemy, c

//------------------------------------------------------------------------------

#ifdef MAPBASE
void CAI_Squad::MarkEnemyAsEluded( CAI_BaseNPC *pUpdater, CBaseEntity *pEnemy )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is gated by a convar, there isn't really any concern about it affecting normal gameplay. But I have to wonder what happens if one NPC in a squad can't see the enemy and another can. Isn't it possible an NPC could be marked as eluded while still being visible by another squad member?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's visible to another squad member, then the squad member will broadcast that to others in the squad and update the enemy's last known position. The eluded code only runs when the NPC is at the enemy's last known position and the enemy is nowhere to be found. I'm reasonably certain those two situations can't happen at once

void ScriptSetSquadData( int iSlot, const char *data );
const char *ScriptGetSquadData( int iSlot );
void ScriptSetSquadData( int iSlot, int data );
int ScriptGetSquadData( int iSlot );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intended purpose of this return type being changed from char* to int?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was one of the points in the PR description:

The VScript functions for squad data now use integers instead of strings, as even though squad data supposedly accepts any parameter, only integers are capable of save/restore. Since these functions were very obscure (and only partially functional), this is unlikely to break existing scripts.

// With this change, these NPCs will establish LOS according to enemy memory instead of where the enemy
// actually is. This may make the NPCs more consistent and fair, but their AI and levels built around it
// may have been designed around this bug, so this is currently being tied to a cvar.
Vector vecEnemy = ( task != TASK_GET_PATH_TO_ENEMY_LKP_LOS || !ai_enemy_memory_fixes.GetBool() ) ? GetEnemy()->GetAbsOrigin() : GetEnemyLKP();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to reread the comment a couple of times before I understood this. Just to be absolutely clear: There is no chance that TASK_GET_PATH_TO_ENEMY_LKP could use this code? Is it worth preserving the original case just to be sure?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is within a switch statement case that is used by TASK_GET_PATH_TO_ENEMY_LOS, TASK_GET_FLANK_RADIUS_PATH_TO_ENEMY_LOS, TASK_GET_FLANK_ARC_PATH_TO_ENEMY_LOS, and TASK_GET_PATH_TO_ENEMY_LKP_LOS. The case for TASK_GET_PATH_TO_ENEMY_LKP is farther above and does not fall back into this code.

if (i == m_SensedObjects.InvalidIndex())
return;

pEntity->RemoveFlag( FL_OBJECT );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does removing this flag from the entity do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It prevents the entity from being sensed anymore. FL_OBJECT's purpose is to mark non-player, non-NPC entities that should be seen by NPC senses.

@Blixibon Blixibon force-pushed the mapbase/feature/npc-memory-and-vscript-expansions branch from beeb68a to 01ad958 Compare January 12, 2025 04:42
@Blixibon
Copy link
Member Author

I've discovered that these changes caused a regression in Combine soldiers and striders. I'm going to explain exactly what went wrong, but tl;dr, I had to add two new cases in npc_combine_s and npc_strider respectively to cover a general design oversight in their schedules. Due to these further behavior changes, and the fact that such issues are possible, I've made the ai_enemy_memory_fixes cvar default to 0 for now.


The issue with Combine soldiers

npc_combine_s uses its own version of SCHED_ESTABLISH_LINE_OF_FIRE which is aptly named SCHED_COMBINE_ESTABLISH_LINE_OF_FIRE.

While SCHED_ESTABLISH_LINE_OF_FIRE tries to find LOS to the enemy's position, SCHED_COMBINE_ESTABLISH_LINE_OF_FIRE tries to find LOS to the enemy's last known position. However, the task used to find the last known position (TASK_GET_PATH_TO_ENEMY_LKP_LOS) accidentally returned the enemy's actual position instead, making them act the same.

This PR fixed the issue with that task, so now SCHED_COMBINE_ESTABLISH_LINE_OF_FIRE behaves differently. Instead of attempting to establish line-of-fire with the enemy's actual position, it attempts to establish line-of-fire with the enemy's last known position. This is what's "supposed" to happen, but there's a problem: The NPC will keep attempting to establish line-of-fire until it actually has line-of-fire with its target, not with its last known position. This means once it establishes line-of-fire with the enemy's last known position, the NPC will get caught in a loop until it's interrupted (e.g. by the enemy coming into view again).

I added a new case for soldiers to use SCHED_COMBINE_PRESS_ATTACK instead of SCHED_COMBINE_ESTABLISH_LINE_OF_FIRE when they can already see the enemy's last known position. SCHED_COMBINE_PRESS_ATTACK causes the soldier to move directly into the enemy's last known position, which breaks them out of the loop. (for reference, NPCs which reach an enemy's last known position and don't see them will mark the enemy as eluded and exit combat)

Other NPCs

After identifying and fixing the issue with Combine soldiers, I searched for every other case of TASK_GET_PATH_TO_ENEMY_LKP_LOS and checked if other NPCs were vulnerable to similar problems. The only other users of this task are npc_metropolice, npc_hunter, and npc_strider.

After testing both npc_metropolice and npc_hunter, I'm fairly confident that they are not vulnerable to this issue. When npc_metropolice tries to establish line-of-fire with a target when it already sees the last known position, it already switches to other schedules that directly advance on that position. npc_hunter very rarely uses this task, and when it does, it falls back to other schedules as well.

npc_strider, however, does get caught in a loop. Instead of having its own line-of-fire schedule, it directly converts TASK_GET_PATH_TO_ENEMY_LOS into TASK_GET_PATH_TO_ENEMY_LKP_LOS. It relies heavily on SCHED_ESTABLISH_LINE_OF_FIRE and doesn't have any alternate advancing schedules like metrocops or hunters do, so it's vulnerable to the same problems as Combine soldiers.

To fix this, I added a similar case where striders would use SCHED_ESTABLISH_LINE_OF_FIRE_FALLBACK when they can already see the enemy's last known position. This falls back to other strider AI used when an enemy is eluded.

@Blixibon Blixibon marked this pull request as ready for review January 25, 2025 05:13
Copy link

@1upD 1upD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking through the PR and talking about it yesterday, I think it looks good to me.

@Blixibon Blixibon merged commit 9efadb7 into mapbase-source:develop Mar 1, 2025
4 checks passed
@Blixibon Blixibon deleted the mapbase/feature/npc-memory-and-vscript-expansions branch March 1, 2025 02:37
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

Successfully merging this pull request may close these issues.

2 participants