Apply radar error to lua spatial queries.

[saurtron]

Work done

• Make lua spatial queries work correctly regarding error position.
  • For: GetUnitsInRectangle, GetUnitsInBox, GetUnitsInCylinder, GetUnitsInSphere, GetUnitsInPlanes

Considerations

I understand this might break backwards compatibility, but it could also not be the case (probably not, please read on :)).

I think it's worth it to review and maybe also review games using this to see if they actually will break (only if they tried to workaround the broken implementation... checked a bit and didn't see any cases, but it's possible there are some).

Even when working around previous behaviour, I think their code would still work, just be doing extra work for nothing.

Also, even if working around this in a breaking way, they will be better off by removing the work around, I guess that can be signalled by exposing the feature capability somehow, so games know engine already has the correct implementation and could prepare in advance.

In case we need to keep backwards compatibility I suppose this has to be controlled through a modinfo switch, or maybe just a new parameter for all those functions. Imo better modinfo since having a parameter just to make the function do the right thing seems a bit awkward.

Anyways let me know what you think is best.

Why?

This automatically fixes users of this functions, like for example bar's settarget circle selection.

The effect is similar to this (the video is from a previous PR, but pre and post situation is basically the same):

Before patch

After patch

[sprunk]

Looks like a large chunk of the PR are refactors (e.g. changing macros from CLuaHandle::GetHandleReadAllyTeam(L) to readAllyTeam defined in the body proper, etc) which makes it harder to see the parts that actually change behaviour. Could the refactors and the logic change be split off into a commit each?

[saurtron]

Looks like a large chunk of the PR are refactors (e.g. changing macros from CLuaHandle::GetHandleReadAllyTeam(L) to readAllyTeam defined in the body proper, etc) which makes it harder to see the parts that actually change behaviour. Could the refactors and the logic change be split off into a commit each?

Well, it's not really a refactor, it's just I need to get CLuaHandle::GetHandleReadAllyTeam(L) into readAllyTeam (for UNIT_ERROR_POS define), so I also changed other occurrences in the same function to not call again and reuse the value, that's all.

Anyways sure, I can split in two commits but it's going to be just a couple of those changing (literally 4 lines in total, 2 at the defines and 2 at GetUnitsInPlanes). ie, it's not a large part at all. I don't think I changed any other lines unless I really needed too due to the way I need to apply a solution.

edit: already done: 0b35f6c

[saurtron]

Anyways, I'll explain the general idea:

Before, we had tests like:

spring/rts/Lua/LuaSyncedRead.cpp

Lines 3069 to 3073 in 27c6bab

	#define CYLINDER_TEST \
	const float3& p = unit->midPos; \
	const float dx = (p.x - x); \
	const float dz = (p.z - z); \
	const float dist = ((dx * dx) + (dz * dz)); \

And then it would loop like:

spring/rts/Lua/LuaSyncedRead.cpp

Line 3086 in 27c6bab

LOOP_UNIT_CONTAINER(VISIBLE_TEAM_TEST, CYLINDER_TEST, true);

Problem here, is in some cases (the ones actually looping enemy units) we need to get the error position.

So, I'm taking pos out of those defines, so they assume p is already defined. Then I create two new defines to compose with the tests, depending on what position code we need:

spring/rts/Lua/LuaSyncedRead.cpp

Lines 2936 to 2942 in 4960869

	#define UNIT_POS \
	const float3& p = unit->midPos;
	#define UNIT_ERROR_POS \
	float3 p = unit->midPos; \
	if (!LuaUtils::IsAllyUnit(L, unit)) \
	p += unit->GetLuaErrorVector(readAllyTeam, fullRead);

So, for example the start of the cylinder test becomes:

spring/rts/Lua/LuaSyncedRead.cpp

Lines 3124 to 3125 in 4960869

	#define CYLINDER_TEST \
	const float dx = (p.x - x); \

And the tests become (when doing just allies):

spring/rts/Lua/LuaSyncedRead.cpp

Line 3141 in 4960869

LOOP_UNIT_CONTAINER(SIMPLE_TEAM_TEST, UNIT_POS CYLINDER_TEST, true);

or (when enemies can appear in the loop):

spring/rts/Lua/LuaSyncedRead.cpp

Line 3143 in 4960869

LOOP_UNIT_CONTAINER(VISIBLE_TEAM_TEST, UNIT_ERROR_POS CYLINDER_TEST, true);

And then we have the helper ApplyPlanarTeamError, and use that to change mins and maxs to pass into quadField.GetUnitsExact(qfQuery, mins, maxs);.

So, as in other similar fixes, we're enlarging the quadfield search to make sure we catch all radar drifted dots. And then in the test for distance we use the radar corrected position.

That's basically it, it's just 4-5 functions with very similar code (imo most of that could be refactored into just one define since it's cloned code but just changing the spatial test).

[saurtron]

Looks like a large chunk of the PR are refactors (e.g. changing macros from CLuaHandle::GetHandleReadAllyTeam(L) to readAllyTeam defined in the body proper, etc) which makes it harder to see the parts that actually change behaviour. Could the refactors and the logic change be split off into a commit each?

There you go: 0b35f6c

[sprunk]

it's going to be just a couple of those changing (literally 4 lines in total, 2 at the defines and 2 at GetUnitsInPlanes). ie, it's not a large part at all. I don't think I changed any other lines unless I really needed too due to the way I need to apply a solution.

Yes, I mean the lines that needed changing to apply the solution as well. There is a lot of this kind of preparation, and splitting it off into separate tiny commits makes it possible to quickly tell "yeah this is a no-op" from a reviewer PoV and be done with it while reducing the commit that contains the actual changes.

As an exercise, here is a branch where I split this PR into chunks ideal from my PoV - all commits except the last one are trivially reviewable and can easily be shown not to affect logic, and that leaves the last one the smallest it can be: https://github.com/beyond-all-reason/spring/commits/refactoring-split-example/

I'm not necessarily asking to make PRs with this level of splitting, but I would generally be happy to get as much as possible out of the way and tiny no-logic-change chunks greatly speed this up.

[saurtron]

As an exercise, here is a branch where I split this PR into chunks ideal from my PoV - all commits except the last one are trivially reviewable and can easily be shown not to affect logic, and that leaves the last one the smallest it can be: https://github.com/beyond-all-reason/spring/commits/refactoring-split-example/

I'm not necessarily asking to make PRs with this level of splitting, but I would generally be happy to get as much as possible out of the way and tiny no-logic-change chunks greatly speed this up.

All right. For next time I know what to do then. Thanks for taking the time to make that branch!

[sprunk]

Looks good, just minor nits

[lhog]

Can you get rid of these ugly macro defs?

[saurtron]

Can you get rid of these ugly macro defs?

not really without a huge refactor, imo that should be left for a future PR

[lhog]

Can you get rid of these ugly macro defs?

not really without a huge refactor, imo that should be left for a future PR

Got it, not a problem.
LGTM then.