bugfix(radar): Fix incorrect 2D distance calculation in Radar::tryEvent

[johnneijzen]

Summary

The 2D distance calculation in Radar::tryEvent was incorrect due to operator precedence.

The previous implementation:

Real distSquared =
    m_event[i].worldLoc.x - pos->x * m_event[i].worldLoc.x - pos->x +
    m_event[i].worldLoc.y - pos->y * m_event[i].worldLoc.y - pos->y;

does not compute the squared distance:

(x1 - x2)^2 + (y1 - y2)^2

Because multiplication has higher precedence than subtraction, the expression is evaluated as:

x1 - (x2 * x1) - x2 + y1 - (y2 * y1) - y2

This can produce incorrect values, including negative results, which are invalid for squared distance.

---

Example

Using:

x1 = 10
y1 = 5
x2 = 3
y2 = 1

Expected result (correct formula)

dx = 10 - 3 = 7
dy = 5 - 1 = 4

distSquared = 7*7 + 4*4
            = 49 + 16
            = 65

Previous implementation result

10 - (3 * 10) - 3 + 5 - (1 * 5) - 1
= 10 - 30 - 3 + 5 - 5 - 1
= -24

Squared distance should never be negative, which demonstrates the calculation was incorrect.

---

Fix

Replaced the calculation with the proper squared distance formula:

Real dx = m_event[i].worldLoc.x - pos->x;
Real dy = m_event[i].worldLoc.y - pos->y;
Real distSquared = dx * dx + dy * dy;

[greptile-apps]

Greptile Summary

This PR fixes a critical operator precedence bug in the 2D distance calculation that could cause incorrect radar event filtering. The original formula incorrectly evaluated multiplication before subtraction, resulting in invalid (potentially negative) squared distance values.

• Replaced buggy inline calculation with correct squared distance formula using intermediate variables
• Added const qualifiers for improved code clarity
• Minor style inconsistency: array indexing spacing differs from file convention

Confidence Score: 4/5

• This PR is safe to merge after addressing the minor style inconsistency
• The mathematical fix is correct and well-explained. The only issue is a minor style inconsistency with array indexing spacing that should be corrected for code consistency
• Core/GameEngine/Source/Common/System/Radar.cpp needs minor style adjustment for array indexing spacing

Important Files Changed

Filename	Overview
Core/GameEngine/Source/Common/System/Radar.cpp	Fixed critical operator precedence bug in 2D distance calculation; minor style inconsistency with array indexing spacing

_{Last reviewed commit: 0ddd87b}

[Caball009]

Nice find. We'll need to check if this needs to go behind the retail compatibility macro.

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[Caball009]

Looks ok.

AFAICT this is only affects local stuff; e.g. control bar, game ui, audio and eva.

[johnneijzen]

so far i can see i don't think we need to add retail compatibility macro because since this affects only player base ui behavior when comes to radar event and spacebar key for example if player discovered stealth unit or "we're under attack" event when player been attacked

[xezon]

Can be simplified in one line.

What was the user facing bug with this?

[xezon]

Real distSquared = sqr(m_event[ i ].worldLoc.x - pos->x) + sqr(m_event[ i ].worldLoc.y - pos->y);

[johnneijzen]

if want one line solution i could rewrite as. removing need of sqr macro/function

const Real distSquared = 
    (m_event[i].worldLoc.x - pos->x) * (m_event[i].worldLoc.x - pos->x) +
    (m_event[i].worldLoc.y - pos->y) * (m_event[i].worldLoc.y - pos->y);

[Skyaero42]

The one line solution look slightly more efficient in Godbolt. Although it has one instruction more, it saves on memory 4 store/reload pairs.

[xezon]

sqr(m_event[i].worldLoc.x - pos->x)

is identical to

(m_event[i].worldLoc.x - pos->x) * (m_event[i].worldLoc.x - pos->x)

[johnneijzen]

Can be simplified in one line.

What was the user facing bug with this?

Not really. The only thing this code would fix is the distance check required for a radar event to be triggered. In most cases, the old formula would just spam events. However, in this particular case, the condition was almost always true because closeEnoughDistanceSq = 250.0f * 250.0f. You probably wont notice it because framesBetweenEvents = LOGICFRAMES_PER_SECOND * 10, so spamming was mostly prevented by the time interval.

The reason I wrote it as three lines is that this is the most common style used when developers calculate distances, for example in Core\GameEngine\Source\GameClient\FXList.cpp and GeneralsMD\Code\Tools\WorldBuilder\src\WaterOptions.cpp.

			// get distance from our new event location to this event location in 2D
			Real distSquared = m_event[ i ].worldLoc.x - pos->x * m_event[ i ].worldLoc.x - pos->x +
												 m_event[ i ].worldLoc.y - pos->y * m_event[ i ].worldLoc.y - pos->y;

			if( distSquared <= closeEnoughDistanceSq )
			{

				// finally only reject making a new event of this existing one is "recent enough"
				if( currentFrame - m_event[ i ].createFrame < framesBetweenEvents )
					return FALSE;  // reject it

			}