Use move for std::vector instead of copying in path functions #1895

bobtista · 2025-11-23T17:02:26Z

Closes Consider moving std::vector instead of copying the data #1891

Refactors path-following functions to use move when transferring std::vector<Coord3D> ownership

Changes:

Updated aiFollowExitProductionPath and aiFollowPath inline functions to accept non-const vector pointers and use std::move
Modified AIStateMachine::setGoalPath to move the vector instead of copying
Updated privateFollowPath and doQuickExit signatures to support move semantics

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

xezon

Did you check that the passed vector is never used after it is consumed?

Generals/Code/GameEngine/Include/GameLogic/AI.h

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

bobtista · 2025-11-25T19:11:14Z

Did you check that the passed vector is never used after it is consumed?

Looking at call sites, they all pass local vectors (exitPath, path) that are not used after the function call. They go out of scope immediately, so this is safe, right?

xezon · 2025-11-25T19:26:10Z

Yes, when the std::vector contents are not used anymore after they are now consumed by the functions then this is safe.

Dependencies/Utility/Utility/CppMacros.h

Generals/Code/GameEngine/Include/GameLogic/AI.h

Dependencies/Utility/Utility/CppMacros.h

xezon · 2025-11-29T12:13:41Z

Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp

 break;
 case AICMD_FOLLOW_PATH:
-privateFollowPath(&parms->m_coords, parms->m_obj, parms->m_cmdSource, FALSE);
+privateFollowPath(&const_cast<AICommandParms*>(parms)->m_coords, parms->m_obj, parms->m_cmdSource, FALSE);


What is going on with the const_cast here? This looks dangerous. Can we get rid of it?

Yeah, it can be mutable. Another option maybe we change aiDoCommand to take non-const pointer, but that changes more stuff. I set it to mutable.

That is not so elegant either.

The problem looks to be that AICommandParms is passed as const to aiDoCommand functions. I suspect we need another change and pass it as non const, UNLESS there is a very good reason why we cannot consume these structures.

This would also allow to optimize this call:

m_pendingCommand.store(*parms)

It currently creates a copy of the whole thing, instead of swapping/moving it.

Perhaps we need another change, refactoring aiDoCommand first, then optimize m_pendingCommand and this change.

#1924, and I used a script to replicate to generals :)

Generals/Code/GameEngine/Include/GameLogic/AIStateMachine.h

xezon · 2025-11-29T16:43:31Z

Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp

 break;
 case AICMD_FOLLOW_PATH:
-privateFollowPath(&parms->m_coords, parms->m_obj, parms->m_cmdSource, FALSE);
+privateFollowPath(&const_cast<AICommandParms*>(parms)->m_coords, parms->m_obj, parms->m_cmdSource, FALSE);


That is not so elegant either.

The problem looks to be that AICommandParms is passed as const to aiDoCommand functions. I suspect we need another change and pass it as non const, UNLESS there is a very good reason why we cannot consume these structures.

This would also allow to optimize this call:

m_pendingCommand.store(*parms)

It currently creates a copy of the whole thing, instead of swapping/moving it.

Perhaps we need another change, refactoring aiDoCommand first, then optimize m_pendingCommand and this change.

xezon · 2025-11-30T19:24:19Z

GeneralsMD/Code/GameEngine/Include/GameLogic/AI.h

 Object*	m_otherObj;
 const Team*	m_team;
-std::vector<Coord3D>	m_coords;
+mutable std::vector<Coord3D>	m_coords;


After thinking about this more, let us do a lightweight refactor and only touch where we can easily avoid a copy without const_casts or mutables.

Then also close #1924

Ok, so close 1924, and close this, and make another PR? Which changes do we want?

Remove the mutable and then keep the copies where it would fail to compile.

Ok I closed 1924 and AICommandParms::m_coords is now non-mutable again.

…owPath

…hange parameter to reference

…llback

xezon · 2025-12-16T22:27:51Z

Generals/Code/GameEngine/Include/GameLogic/AI.h

 }

-void aiFollowExitProductionPath( const std::vector<Coord3D>* path, Object *ignoreObject, CommandSourceType cmdSource )
+inline void aiFollowExitProductionPath( std::vector<Coord3D>* path, Object *ignoreObject, CommandSourceType cmdSource )


Adds inline keyword. Multiple times.

xezon · 2025-12-16T22:29:16Z

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

-m_coords = parms.m_coords;
+// We intentionally const_cast here so we can move the path coordinates into storage.
+// AICommandParms is treated as a transient command container that is not reused after dispatch.
+std::vector<Coord3D>& coords = const_cast<std::vector<Coord3D>&>(parms.m_coords);


Is it worth it to do this? Otherwise I recommend to leave the copy. Is safer if something changes higher up.

xezon · 2025-12-16T23:04:22Z

Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp

 case AICMD_FOLLOW_PATH:
-privateFollowPath(&parms->m_coords, parms->m_obj, parms->m_cmdSource, FALSE);
+{
+// Keep AICommandParms const; use a local copy of the coordinates when following the path.


This comment is a bit confusing.

Caball009 reviewed Nov 24, 2025

View reviewed changes

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp Outdated Show resolved Hide resolved

bobtista marked this pull request as ready for review November 25, 2025 17:58

xezon reviewed Nov 25, 2025

View reviewed changes

Generals/Code/GameEngine/Include/GameLogic/AI.h Outdated Show resolved Hide resolved

Generals/Code/GameEngine/Include/GameLogic/AI.h Outdated Show resolved Hide resolved

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp Outdated Show resolved Hide resolved

xezon added Minor Severity: Minor < Major < Critical < Blocker Performance Is a performance concern labels Nov 25, 2025

xezon added this to the Runtime performance fixes & optimizations milestone Nov 25, 2025

xezon reviewed Nov 25, 2025

View reviewed changes

Dependencies/Utility/Utility/CppMacros.h Outdated Show resolved Hide resolved

Dependencies/Utility/Utility/CppMacros.h Outdated Show resolved Hide resolved

Generals/Code/GameEngine/Include/GameLogic/AI.h Outdated Show resolved Hide resolved

xezon reviewed Nov 29, 2025

View reviewed changes

bobtista mentioned this pull request Nov 29, 2025

refactor(perf): change aiDoCommand to take non-const pointer for move semantics #1924

Closed

xezon reviewed Nov 30, 2025

View reviewed changes

bobtista added 13 commits December 15, 2025 10:08

Use move semantics in aiFollowPath and aiFollowExitProductionPath

28dc02a

Use move semantics in AIStateMachine::setGoalPath

d1f0ceb

Update privateFollowPath to use move semantics

ac221e5

Use swap instead of move for VC6 compatibility

c329b8b

Use const_cast in aiDoCommand to pass non-const vector to privateFoll…

d60c161

…owPath

Add conditional compilation for move vs swap and clear source vector

f5f4cc6

nit: make comments consistent

63fc198

refactor(perf): centralize move semantics helper in CppMacros.h

e1d88fd

refactor: remove unnecessary CppMacros.h includes

7aa82ff

refactor(perf): rename move_assign_from_pointer to move_or_swap and c…

5808da2

…hange parameter to reference

refactor(perf): remove comment format and fix C++03 move semantics fa…

694944d

…llback

refactor(perf): make m_coords mutable to remove const_cast

d5da54b

remove include

8c112de

bobtista force-pushed the bobtista/move-vector-instead-of-copy branch from e97db76 to 8c112de Compare December 15, 2025 15:17

bobtista added 3 commits December 15, 2025 10:25

refactor(perf): move coords into AICommandParmsStorage via const_cast

44d2fa1

refactor(perf): remove mutable coords from AICommandParms

803e5b3

refactor(perf): keep AICommandParms const and copy coords where needed

e9b7e5c

xezon reviewed Dec 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use move for std::vector instead of copying in path functions #1895

Use move for std::vector instead of copying in path functions #1895

Uh oh!

bobtista commented Nov 23, 2025

Uh oh!

xezon left a comment

Uh oh!

Uh oh!

Uh oh!

bobtista commented Nov 25, 2025

xezon commented Nov 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xezon Nov 29, 2025

bobtista Nov 29, 2025

xezon Nov 29, 2025

bobtista Nov 29, 2025

Uh oh!

xezon Nov 29, 2025

xezon Nov 30, 2025

bobtista Nov 30, 2025

xezon Dec 1, 2025

bobtista Dec 15, 2025

xezon Dec 16, 2025

xezon Dec 16, 2025

xezon Dec 16, 2025

Labels

3 participants

Use move for std::vector instead of copying in path functions #1895

Are you sure you want to change the base?

Use move for std::vector instead of copying in path functions #1895

Uh oh!

Conversation

bobtista commented Nov 23, 2025

Changes:

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bobtista commented Nov 25, 2025

xezon commented Nov 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

3 participants