refactor: move /take and /give to LUA by keithharvey · Pull Request #2423 · beyond-all-reason/RecoilEngine

keithharvey · 2025-07-01T06:50:20Z

Work done

Add TakeActionExecutor class to handle /take command forwarding
Register /take command in synced_commands.json to forward to Lua
Similarly, /give was moved to LUA
Remove old C++ /take command implementation
Improve modularity by delegating command logic to Lua gadgets
introduced a reason pass through for both ChangeTeam and AllowUnitTransfer, with the engine. The engine retains knowledge of only the ChangeTeam actions it initiates with its own enum.

This change allows the /take command to be implemented in Lua for better modularity and easier maintenance, while maintaining the same user interface. It also adds a reason to pass through to AllowUnitTransfer.

Testing

Ensured /take still worked in skirmish

sprunk

Needs a replacement Lua gadget to be put into examples or basecontent.

keithharvey · 2025-07-03T01:55:22Z

Needs a replacement Lua gadget to be put into examples or basecontent.

This was our conversation about a proxy to preserve /take or am I misunderstanding? I believe this comment is resolved.

keithharvey · 2025-07-03T01:56:11Z

Removing draft from this PR since I believe it is now ready for testing/review.

- Add TakeActionExecutor class to handle /take command forwarding - Register /take command in synced_commands.json to forward to Lua - Remove old C++ /take command implementation - Improve modularity by delegating command logic to Lua gadgets This change allows the /take command to be implemented in Lua for better modularity and easier maintenance, while maintaining the same user interface.

Gave C++ its own enum for only the transfer types it is responsible for. LUA handles the rest and C++ simply passes everything through the appropriate hooks

badosu · 2025-07-09T15:16:01Z

 int LuaSyncedCtrl::TransferUnit(lua_State* L)
 {
 	CheckAllowGameChanges(L);
+	const int args = lua_gettop(L);


This is a breaking change, in general we avoid breaking changes.

We'd instead make a new overload (in the documentation) that accepts the new type, and mark the previous as deprecated.

The old method would still work.

This function should be 100% untouched.

So I do think we should eventually deprecate the existing hook. capture is a useless parameter compared with reason.

It can be deprecated when capture handling is moved to Lua.

badosu · 2025-07-09T15:16:43Z

 #include "Sim/Units/Scripts/LuaUnitScript.h"

+// for profiler
+#include "System/TimeProfiler.h"


Is this necessary?

No sorry this was a me profiling during testing.

badosu · 2025-07-09T15:17:33Z

-		// capture failed
-		if (team == gu->myTeam) {
-			LOG_L(L_WARNING, "%s: Capture failed, unit type limit reached", unitDef->humanName.c_str());
-			eventHandler.LastMessagePosition(pos);


Is the removal of a callin invocation intended? (the new one does not contain it)

Removing LastMessagePosition is probably ok, adding UnitTaken isn't. Generally leave the UnitBla callins as they were.

badosu · 2025-07-09T15:20:14Z

      "description" : "Fast-forwards to a given frame, or stops fast-forwarding"
   },
-   "take" : {
+   "spectator" : {


these two files are automatically generated, they are only committed within the repository to provide some default data to site (when the user does not perform the task to refresh them).

badosu · 2025-07-09T15:22:17Z

-class GiveActionExecutor : public ISyncedActionExecutor {
-public:
-	GiveActionExecutor() : ISyncedActionExecutor(
-		"Give",


Mark it as deprecated in the description recommending the alternative instead.

badosu · 2025-07-09T15:22:25Z

-	TakeActionExecutor() : ISyncedActionExecutor(
-		"Take",
-		"Transfers all units of allied teams without any "
-		"active players to the team of the issuing player"


Mark it as deprecated in the description recommending the alternative instead.

sprunk · 2025-07-10T16:38:46Z

+	// For the full list of reasons, see 'GG.CHANGETEAM_REASON' in luarules/gadgets.lua
+	// The two enums should be kept in sync for shared values.
+	enum class ChangeTeamReasonCpp {
+		RECLAIMED = 0,


"Reclaim" is already the term for gathering resources from wrecks.

I don't think this needs to introduce a new value since the engine shouldn't do anything about it anyway.

sprunk · 2025-07-10T16:41:12Z

-		ChangeGiven,
-		ChangeCaptured
+	// C++-side reasons for changing team.
+	// For the full list of reasons, see 'GG.CHANGETEAM_REASON' in luarules/gadgets.lua


Engine generally shouldn't refer to Lua (even if it provides examples of its own). This is because Lua is 100% replaceable (i.e. there may not be GG.CHANGETEAM_REASON, a GG table, or even gadgets.lua for any particular game). If Lua gets to define something then it's none of engine's business, and if engine has to define it then that is the source of truth and it is Lua who should refer to the engine.

Hmm, yeah. It is just a pass through, but the engine does refer to this enum a few places and the values match. But your point is taken. What is the best way to expose this to LUA so it is upstream.

The enum should be kept as-is, AFAICT its only use is to pick which of the two irrelevant stats this affects, "units sent" or "units outcaptured". These stats are a larger topic, see #2041 but the tl;dr is that we can't add a 3rd stat anyway for protocol reasons, and they would ideally be moved to Lua sometime anyway.

RecoilEngine/rts/Sim/Misc/Team.cpp

Lines 440 to 445 in a9bd9de

case RemoveGiven: {

GetCurrentStats().unitsSent++;

} break;

case RemoveCaptured: {

GetCurrentStats().unitsOutCaptured++;

} break;

Lua doesn't need to have the reason passed via the callin, see #2423 (comment)

sprunk · 2025-07-10T16:46:01Z

 {
 	ZoneScoped;
-
-	bool allow = true;


result.first and result.second are much worse names than allow and drop.

sprunk · 2025-07-10T16:48:38Z

+	if (oldTeam == newTeam)
+		return true;


Is this just for the factory buildee transfer?

I put it in a state with logging in it where I noticed it was getting called a LOT, so wanted to add a guard clause to save some pointless work.

sprunk · 2025-07-10T16:49:28Z

+ * @return false to disallow unit transfer
 */
-bool CSyncedLuaHandle::AllowUnitTransfer(const CUnit* unit, int newTeam, bool capture)
+bool CSyncedLuaHandle::AllowUnitTransfer(const CUnit* unit, int newTeam, int reason)


This function should also be 100% untouched.

hmm, that was a key reason for this patch. It allows us to have better information in downstream widgets to make decisions about allowing unit transfer.

How about this? Should work without any engine changes.

-- take_gadget.lua function HandleTakeRequest() -- ... Spring.SetUnitRulesParam(unitID, "transfer_reason", "take") Spring.TransferUnit(unitID, teamID) Spring.SetUnitRulesParam(unitID, "transfer_reason", nil)

-- another_wupget.lua function wupget:UnitTaken(unitID, ..., capture) -- or gadget:AllowUnitTransfer local reason = Spring.GetUnitRulesParam(unitID, "transfer_reason") if reason == "take" then -- ... elseif reason == "betrayal" then -- ... elseif reason == nil then if capture then -- engine capture mechanic else -- transfer request from a player end -- alternatively, a very early layer gadget could inject these two as reasons in AllowTransfer if it doesn't find any, so that it's never nil anywhere else end end

If you want a master list accessible to both widgets and gadgets, do something like this

-- master_list_gadget.lua function Initialize() -- not yet supported, see engine #1057 -- Spring.SetGameRulesParam("transfer_reasons", {"take", "betrayal"}) -- current workaround, manual enumeration Spring.SetGameRulesParam("transfer_reasons_n", 2) Spring.SetGameRulesParam("transfer_reasons_1", "take") Spring.SetGameRulesParam("transfer_reasons_2", "betrayal")

sprunk · 2025-07-10T16:59:56Z

Needs a replacement Lua gadget to be put into examples or basecontent.

This was our conversation about a proxy to preserve /take or am I misunderstanding? I believe this comment is resolved.

Sorry, what I meant was that something similar to cmd_take.lua from your beyond-all-reason/Beyond-All-Reason#5446 should be put into basecontent, except with no BAR specific code (at a glance this means no GG.CHANGETEAM_REASON, the rest of it looks kosher).

This allows building on Apple Silicon Macs by automatically detecting ARM64 and using --platform linux/amd64 for Docker commands.

sprunk · 2025-08-07T10:27:46Z

TeamTransferWithReason can be (and should be, because the engine doesn't do anything meaningful with the reason) in Lua, see #2423 (comment). At the moment I don't think there's any engine change needed to add Lua /take.

I figure we want it to match the actual best-implementation we implement in bar. Should probably include the gadget hander stuff

- Remove prompts/team_transfer/ directory (13,341+ lines of explanatory content) - Remove examples/lua2cpp_example.lua and lua2cpp_output.cpp (auto-generated examples) - Remove cont/base/springcontent/LuaGadgets/Gadgets/team_transfer.lua (example implementation) - Preserve core team transfer deprecation/fallback functionality in C++ files - Keep auto-generated synced_commands.json changes for take/capture commands This creates a clean functional branch with only the intentional team transfer deprecation and fallback changes, removing all explanatory and example content. Co-Authored-By: Keith Harvey <keithdanielharvey@gmail.com>

cleanup: remove noise files and auto-generated content

keithharvey · 2025-08-14T03:56:12Z

@@ -133,6 +133,9 @@ progress.make
 /installer/Springlobby/
 /installer/*.exe



remove this

keithharvey · 2025-08-14T03:56:36Z

@@ -266,11 +266,12 @@ end
 --------------------------------------------------------------------------------
 --------------------------------------------------------------------------------


revert all of the cont/base/springcontent changes

keithharvey · 2025-08-14T03:59:16Z

@@ -1137,9 +1137,9 @@ function gadgetHandler:AllowUnitCreation(unitDefID, builderID, builderTeam, x, y
 end




I think we just delete this. We don't need it downstream in BAR. SO just keep the deprecated call and delete this reason based one.

keithharvey · 2025-08-14T22:21:21Z

My hunt for ChangeTeamReason led me to realize all of these code paths were related and that all transfers should be handled by LUA. This simplifies things greatly on the engine side, and doing it piecemeal seemed harder since the test grid was already huge, even for extremely limited engine changes to transfers.

Closing this in favor of the new PR, which has the new focus: #2528

keithharvey changed the title ~~feat: Add TakeActionExecutor for /take command forwarding to Lua~~ feat: Move /take to Lua Jul 1, 2025

keithharvey mentioned this pull request Jul 1, 2025

move /take to LUA beyond-all-reason/Beyond-All-Reason#5446

Closed

keithharvey marked this pull request as draft July 1, 2025 06:55

sprunk requested changes Jul 1, 2025

View reviewed changes

Comment thread rts/Lua/LuaHandleSynced.cpp

Comment thread rts/Game/SyncedGameCommands.cpp Outdated

keithharvey marked this pull request as ready for review July 3, 2025 01:55

keithharvey added 5 commits July 2, 2025 19:56

pass-through transfer reason

b516fd1

Gave C++ its own enum for only the transfer types it is responsible for. LUA handles the rest and C++ simply passes everything through the appropriate hooks

remove erroneous changes

879205d

further reduce calls

38122d9

further cleanup and reverts

ccb3960

keithharvey force-pushed the move_take branch from baef62b to ccb3960 Compare July 3, 2025 01:56

keithharvey changed the title ~~feat: Move /take to Lua~~ refactor: ChangeTeam/AllowUnitTransfer, move /take and /give to LUA Jul 3, 2025

keithharvey changed the title ~~refactor: ChangeTeam/AllowUnitTransfer, move /take and /give to LUA~~ refactor: reason added to ChangeTeam/AllowUnitTransfer, move /take and /give to LUA Jul 3, 2025

keithharvey requested a review from sprunk July 3, 2025 02:12

keithharvey changed the title ~~refactor: reason added to ChangeTeam/AllowUnitTransfer, move /take and /give to LUA~~ refactor: ChangeTeam/AllowUnitTransfer simplification, move /take and /give to LUA Jul 3, 2025

badosu reviewed Jul 9, 2025

View reviewed changes

sprunk reviewed Jul 10, 2025

View reviewed changes

Comment thread doc/site/data/unsynced_commands.json Outdated

sprunk reviewed Jul 10, 2025

View reviewed changes

Comment thread cont/base/springcontent/LuaGadgets/Gadgets/share_delayed.lua Outdated

keithharvey marked this pull request as draft August 5, 2025 21:47

Daniel Harvey and others added 3 commits August 6, 2025 19:31

feat(docker-v2): Add docker-platform flag for Apple Silicon support

843a9ac

This allows building on Apple Silicon Macs by automatically detecting ARM64 and using --platform linux/amd64 for Docker commands.

wip team_transfer_bar

219893d

wip: team transfer example

d95372f

keithharvey changed the title ~~refactor: ChangeTeam/AllowUnitTransfer simplification, move /take and /give to LUA~~ refactor: team transfer Aug 7, 2025

keithharvey and others added 5 commits August 8, 2025 01:39

team_transfer example

8a2a258

I figure we want it to match the actual best-implementation we implement in bar. Should probably include the gadget hander stuff

wip: more module work, resources

cf69d61

wip plz dont read

8b4a729

Merge pull request #1 from keithharvey/move_take_cleaned

1b72d58

cleanup: remove noise files and auto-generated content

keithharvey commented Aug 14, 2025

View reviewed changes

keithharvey changed the title ~~refactor: team transfer~~ refactor: move /take and /give to LUA Aug 14, 2025

keithharvey closed this Aug 14, 2025

sprunk mentioned this pull request Aug 15, 2025

feat: deprecate team transfer, add sharing hooks #2528

Closed

keithharvey mentioned this pull request Aug 20, 2025

refactor(TeamTransfer): minimal framework gadget and policy migration (sharing_tab) keithharvey/bar#6

Closed

6 tasks

	case RemoveGiven: {
	GetCurrentStats().unitsSent++;
	} break;
	case RemoveCaptured: {
	GetCurrentStats().unitsOutCaptured++;
	} break;

		@@ -133,6 +133,9 @@ progress.make
		/installer/Springlobby/
		/installer/*.exe

		@@ -266,11 +266,12 @@ end
		--------------------------------------------------------------------------------
		--------------------------------------------------------------------------------

		@@ -1137,9 +1137,9 @@ function gadgetHandler:AllowUnitCreation(unitDefID, builderID, builderTeam, x, y
		end

Uh oh!

Conversation

keithharvey commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work done

Testing

Uh oh!

sprunk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keithharvey commented Jul 3, 2025

Uh oh!

keithharvey commented Jul 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sprunk Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithharvey Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sprunk commented Jul 10, 2025

Uh oh!

sprunk commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithharvey commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

keithharvey commented Jul 1, 2025 •

edited

Loading

sprunk Jul 10, 2025 •

edited

Loading

keithharvey Jul 25, 2025 •

edited

Loading

keithharvey commented Aug 14, 2025 •

edited

Loading