move /take to LUA

[keithharvey]

Required Engine PR

beyond-all-reason/RecoilEngine#2423

Work done

• Create cmd_take.lua gadget implementing /take command functionality
• Move all give logic into cmd_give
• ChangeTeam gets a new reason parameter, mostly managed by a LUA enum which is comprehensive, with a subset of functionality the engine handled defined in a new enum
• AllowUnitTransfer gets this reason from the engine pass through, allowing more sensible handling of the parameter beyond "capture". For example, this allows fixing the /take bug in sharing code easily
• some code cleanup/fixes

Test steps

The /take command now works reliably in skirmish mode, transferring units and resources from inactive AI teams to human players without Lua errors

[sprunk]

Will need changes due to beyond-all-reason/RecoilEngine#2423.

[WatchTheFort]

Why is the gadget global table GG being localized? That table is huge, what benefit are we incurring?

local isSharing = reason == GG.CHANGETEAM_REASON.GIVEN or reason == GG.CHANGETEAM_REASON.IDLE_PLAYER_TAKEOVER or reason == GG.CHANGETEAM_REASON.TAKEN or reason == GG.CHANGETEAM_REASON.SOLD

This is used in a lot of places, can it be refactored to be defined in a single location?

Why are oldTeam == newTeam checks being added in AllowUnitTransfer callins? When that is the case, it makes no difference what the return value is.

[WatchTheFort]

How can a unit change teams by being reclaimed?

[sprunk]

Please refer to Spring.GetSpiritRealmTeamID()

[WatchTheFort]

Why do these gadgets need to register custom reasons? Why can't they use a generic base set of reasons?

[sprunk]

Widgets might want to know the specific reason, e.g. to produce different notifications.

Caution

UNIT LOST TO SCAVENGER CLOUD!!! <click to center>

vs

Note

Unit X bought for Y metal.

[WatchTheFort]

How do new reasons get added? I'm not crazy about gadgets.lua containing functionality related to a specific gadget.

[badosu]

gadgetHandler.RegisterGlobal(self, "RegisterUnitChangeTeamReason", fn(owner, reason) ->
  reasons[reason] = owner
  flattenedReasons = table_keys(reasons)
end)
gadgetHandler.RegisterGlobal(self, "UNIT_CHANGE_TEAM_REASONS", fn() -> flattenedReasons end)

That would be one idea. Once offloaded to a gadget it's easier to reason about a reasonable interface.

In fact, the specification above incentivizes a straightforward natural extension for reason watching, e.g.:

local function changedTeamsDueToX(...context)
  doSomethingMyGadgetCaresAboutWhenX(...context)
end

function gadget:Initialize()
  -- alternatively just pass "X"
  RegisterUnitChangedTeams(UNIT_CHANGE_TEAM_REASONS.X, changedTeamsDueToX)
end

[keithharvey]

@WatchTheFort there would need to be a functional difference in behavior in game but the current list should be a reductive categorization of existing behaviours, just less obfuscated. I'll move it to a different file.

[WatchTheFort]

Put these on the same line as the other parameters, as was done with all the other callins

[WatchTheFort]

Why was this call removed?

[WatchTheFort]

Why is this file part of the PR?

[WatchTheFort]

Why is this file part of the PR?

[WatchTheFort]

Why is this file part of the PR?

[WatchTheFort]

What is this? Why would Chobby have any take functionality?

[badosu]

I'll avoid extensive review, I'll just give a strong recommendation, feel free to resolve this, I have no stakes except with Recoil consumers having maintainable widget/gadget managers.

Keep in mind this is an opinionated decision to make, one that is based on my experience in my previous life as a BAR contributor and a current Recoil contributor. There is nothing wrong with having manager code being as complex as a game wants, it's game code nevertheless. But there is a strong reason for encapsulating concerns so the codebase is more maintainable and less bug-prone, especially as most BAR contributors avoid touching manager code, due to it's complex nature.

---

Strongly consider having a gadget to handle taking/given logic instead of having this logic in the manager:

• Have a gadget with handler = true on a very low layer
• Move the GG definitions there, alternatively use gadgetHandler.RegisterGlobal(self, "CHANGETEAM_REASON", {...}) instead. If you wish to retain GG use gadgetHandler.GG for the assignment.
• Have the gadget implement all the callins specifically concerned with taking/giving units. Keep in mind when handler = true the gadgetHandler global is the same gadgetHandler as the one here. Some logic such as having the entry in callinslist registered might need to be kept in the manager, as it might be in the limit of what is reasonable to require in an initial refactoring effort.

There might be some other steps I didn't think of more deeply that require careful introspection and understanding of the gadgetHandler to implement above, but BAR heavily benefits of having a more maintainable gadget manager and I highly recommend trying to understand how it works so the implementation works with it instead of fighting against it.

The other important aspect is that having a first gadget implementing offload of callin concerns also incentivizes further manager level BAR logic to do the same, since there's at least one reference implementation. It also nudges BARs contributor base to attempt developing more maintainable and less bug-prone patterns.

If you wish to attempt this route, I can assist, but I won't specifically be reviewing this PR. I'd encourage to reach me at #lua-scripting in the recoil discord server but I don't mind BAR discord.

Example for illustration purpose

An example of "non-vanilla" manager change in BAR that can/might be offloaded to a gadget: AllowCommand watching. There's more code associated to it, but it's just and example.

An example of "non-vanilla" manager change in BAR that is partly offloaded to a widget: VisibleUnit* callins. This one could still be refactored to not need code in manager but at least it keeps its changes minimal. A tangent argument is that this specific widget should be an unsynced gadget, sending messages to LuaUI, since it's a major part of BARs unsynced infrastructure.

[sprunk]

I'm not so sure about handler = true. If callins can be overridden then it can become hard to tell what the actual callout function signature for individual gadgets really is. Transparent wrappers that do stuff but keep args semantics intact when passing to gadgets (such as the wrapper for AllowCommand) are kosher, as are completely new callins (such as VisibleUnitsChanged), but I'd argue that if both the handler and the engine source says the args for some callin are unitID, bool captured but gadgets actually receive unitID, int reason because some obscure gadget override it it's less maintainable than having a few explicit lines directly in the handler that are just not present in the wider Recoil.

[badosu]

It is a good argument, but it's not an argument against offloading of ad-hoc handler concerns to addons (gadgets, widgets), it's an argument against having complex callin boilerplate, which is something that BAR already does and probably will keep doing.

Having handler be as thin and transparent as possible between engine -> handler -> addons is a concern that is independent of encapsulation. My proposal is specifically applied to the fact BAR already intends to not have as much of a thin wrapper as possible.

My argument is that if a game has scoped custom logic that deviates from mainstream managers, it's much better to have this custom logic extracted to a place where it's evident what changes exist and contains what exactly are these changes. In fact, the pattern improves understanding of how a game deviates from standard callin handling (be it from other games via discrepancy to basecontent manager or from the engine's own callin signature) by encapsulating the games ad-hoc changes ¹.

My argument is that this mostly improves maintainability, if your decision is not to have too thin wrappers anyway. There is a slight con on figuring out where callin dispatch is defined, but perhaps having callin_*.lua filename would help on that regard.

I'd argue that if the handler says the args are unitID, bool captured but gadgets actually receive unitID, int reason because some obscure gadget override the callin it's less maintainable than having a few explicit lines directly in the handler that are just not present in the wider Recoil.

Sure, but that is also unrelated to encapsulation. Nothing precludes sensible consideration of callin signature on offloaded callin pattern. Unexpected signature can be done on manager code or on offloaded callin gadget code, it's not an intrinsic problem with the pattern.

Footnotes

1. A practical example: BAR makes extensive use of lua language server annotations. The encapsulated logic assists in defining the types gadgets are expected to have defined in their scope as well, instead of having them spread around in the manager code, in a way conducive to bugs. ↩

[keithharvey]

I think this is a good time for a functional approach to this problem. I am currently deprecating the Spring side where appropriate and moving everything that touches CHANGE_REASON (including capture), but this is a good time to code what you're talking about in exactly 1 place based on reason.

I want to also make a permanently open (or deprecation?) PR cleaning the dead code, for when we eventually decide it's time to end the deprecation, as part of this PR. With that we can make a test grid for (1) new lua implementation, with updated engine (2) bar on master with engine running fallbacks (3) post-deprecation removals everything works fine/our lua implementation is guaranteed complete

[keithharvey]

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 greatly, 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: #5667