Update transporters to be more effective with the area load command.

[lostsquirrel1]

Adds a unitDef to allow transporters to pick up units without trying to turn to face the same heading.

Fixes #1945

[sprunk]

What about the other hardcoded behaviours (e.g. set altitude to target unit height, what if I'd rather the unit was beamed up via tractor beams like an UFO?)
What about these hardcoded constants?

spring/rts/Sim/Units/CommandAI/MobileCAI.h

Lines 112 to 113 in b4240c3

	static constexpr float AIRTRANSPORT_DOCKING_RADIUS = 16.0f;
	static constexpr float AIRTRANSPORT_DOCKING_ANGLE = 50.0f;

---

gadget:AllowUnitTransportLoad can already implement whatever arbitrary checks the game desires, so how about:

• a bool that turns off all behaviours (i.e. rotating heading to match the target, set wanted altitude, set allow landing to false, maxdrift = 1.0f) and instead makes it so that the engine only handles moving inside loadRadius and then relegates everything to the game by starting to call gadget:AllowUnitTransportLoad when inside the radius.
• pass the existing logic-related vars (heading diff, position) to gadget:AllowUnitTransportLoad if not there already, to facilitate games building upon the defaults.

[lostsquirrel1]

What about the other hardcoded behaviours (e.g. set altitude to target unit height, what if I'd rather the unit was beamed up via tractor beams like an UFO?) What about these hardcoded constants?

spring/rts/Sim/Units/CommandAI/MobileCAI.h

Lines 112 to 113 in b4240c3

	static constexpr float AIRTRANSPORT_DOCKING_RADIUS = 16.0f;
	static constexpr float AIRTRANSPORT_DOCKING_ANGLE = 50.0f;

gadget:AllowUnitTransportLoad can already implement whatever arbitrary checks the game desires, so how about:

* a bool that turns off all behaviours (i.e. rotating heading to match the target, set wanted altitude, set allow landing to false, maxdrift = 1.0f) and instead makes it so that the engine only handles moving inside loadRadius and then relegates everything to the game by starting to call `gadget:AllowUnitTransportLoad` when inside the radius.

* pass the existing logic-related vars (heading diff, position) to `gadget:AllowUnitTransportLoad` if not there already, to facilitate games building upon the defaults.

Those values are only used as inputs in the Lua callback AllowUnitTransportLoad, which to my knowledge is ignored by every game. The engine does NOT use the values itself. I've raised a question with the game devs whether any use it. If not, then it would make more sense to remove them completely. These values are there to allow games to keep the original engine behaviour (prior to the inclusion of AllowUnitTransportLoad)

[sprunk]

Those values are only used as inputs in the Lua callback AllowUnitTransportLoad, which to my knowledge is ignored by every game.

They aren't, they are completely lost in a bunch of steps. First the raw values most useful for games (heading diff, distance) aren't passed anywhere, they are only used to produce the "is above arbitrary threshold" bools.

spring/rts/Sim/Units/CommandAI/MobileCAI.cpp

Lines 1408 to 1410 in b4240c3

	const bool isInRange = (owner->pos.SqDistance(wantedPos) < Square(AIRTRANSPORT_DOCKING_RADIUS));
	const bool isAligned = (std::abs(owner->heading - unit->heading) < AIRTRANSPORT_DOCKING_ANGLE);
	const bool isUpright = (owner->updir.dot(UpVector) > 0.995f);

The bools aren't ever used separately, they are ANDed together into a single bool.

spring/rts/Sim/Units/CommandAI/MobileCAI.cpp

Line 1412 in b4240c3

if (!eventHandler.AllowUnitTransportLoad(owner, unit, wantedPos, isInRange && isAligned && isUpright))

That bool is passed to the C++ side of the callin (called allowed below), but never lua_pushboolean'd there.

spring/rts/Lua/LuaHandleSynced.cpp

Lines 864 to 879 in b4240c3

	// use engine default if callin does not exist
	if (!cmdStr.GetGlobalFunc(L))
	return allowed;
	lua_pushnumber(L, transporter->id);
	lua_pushnumber(L, transporter->unitDef->id);
	lua_pushnumber(L, transporter->team);
	lua_pushnumber(L, transportee->id);
	lua_pushnumber(L, transportee->unitDef->id);
	lua_pushnumber(L, transportee->team);
	lua_pushnumber(L, loadPos.x);
	lua_pushnumber(L, loadPos.y);
	lua_pushnumber(L, loadPos.z);
	if (!RunCallIn(L, cmdStr, 9, 1))
	return true;

So Lua does not actually receive any of those. It would be nice if it received number headingDiff and number distance, though it can already calculate these itself by polling the relevant units for position/heading so it's not that important, just inconvenient.

The engine does NOT use the values itself. I've raised a question with the game devs whether any use it. If not, then it would make more sense to remove them completely.

The boolean check(s) are used as the default, in case Lua does not return anything. So they must stay, at least for now. But indeed they aren't used if Lua returns something, so I now think it's fine if they are arbitrarily hardcoded since Lua can just return something if it doesn't like them.

The game can do nothing about the other behaviours though (set altitude, set wanted rotation etc) so there would ideally be a way to opt out of those.

[lostsquirrel1]

Thanks for the clarification that the Lua event handler doesn't push the allowed parameter.

How is the allowed parameter being used as the default value for the call? As far as I see, maybe I'm missing something, CEventHandler::AllowUnitTransportLoad calls ControlIterateDefTrue which always defaults to true - I can't see how it uses allowed as the default value for the return.

[sprunk]

calls ControlIterateDefTrue which always defaults to true

This is only the default if there is no C++ side event client. If there are event clients, any of them returning false is sufficient to make the whole call return false thanks to the &= operator:

spring/rts/System/EventHandler.cpp

Lines 214 to 219 in b4240c3

	bool result = true;
	for (size_t i = 0; i < list.size(); ) {
	CEventClient* ec = list[i];
	result &= (ec->*func)(std::forward<A>(args)...);

The C++ side of LuaHandleSynced::AllowUnitTransportLoad returns the default itself, if Lua didn't:

spring/rts/Lua/LuaHandleSynced.cpp

Lines 881 to 884 in b4240c3

	// ditto if it does but returns nothing
	const bool allow = luaL_optboolean(L, -1, allowed);
	lua_pop(L, 1);
	return allow;

[lostsquirrel1]

Gotcha.

[lostsquirrel1]

In that case I'll leave it for now.

[sprunk]

Having thought about it, the PR is basically acceptable already, perhaps except it would be nice to name the new unit def tag something futureproof like useBuiltinLoadingBehaviours that will be seamlessly extensible to be the single toggle that means "please let me implement my own game design, thank you" when the engine also decides to let games control the other hardcoded behaviours like setting altitude. Not a big deal either way though.