I'd recommend to either nuke the newline or do the jump prior, move the atan to non-mutating function in vector.js (so people don't have to lookup which formula computed the angle again, so that people don't have to lookup what atan2 does again and so that it becomes a bit shorter) and extend the test_vector.js (let me commit a split those scopes prior to that though).

Not sure what atan2(0,0) is

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/atan2

Math.atan2(±0, -0); ±PI.
Math.atan2(±0, +0); ±0.

Wasn't this done to avoid units crossing walls?

Ah, never mind, I didn't read this correctly. Bar your comment this is fine then, and imo correctly does not rely on unitMotion (which has a faceSomething function in svn iirc)

In D1037#40919, @temple wrote:

In addition to those in the patch (and in maps), I found a couple more places where atan2 was used:

components/UnitMotionFlying.js:		var targetAngle = Math.atan2(this.targetX - pos.x, this.targetZ - pos.z);
helpers/Walls.js:	let angle = -Math.atan2(dir.z, dir.x);

But targetX/Z isn't a position so I didn't want to mess with that, and walls is.. I don't know, so I didn't want to mess with that either.

Sure, you are allowed to cleanup these places to use Vector2D separately :P

In D1037#40921, @elexis wrote:

In D1037#40919, @temple wrote:

In addition to those in the patch (and in maps), I found a couple more places where atan2 was used:

components/UnitMotionFlying.js:		var targetAngle = Math.atan2(this.targetX - pos.x, this.targetZ - pos.z);
helpers/Walls.js:	let angle = -Math.atan2(dir.z, dir.x);

But targetX/Z isn't a position so I didn't want to mess with that, and walls is.. I don't know, so I didn't want to mess with that either.

Sure, you are allowed to cleanup these places to use Vector2D separately :P

My point is the first one isn't a position, I'd have to create { 'x': this.targetX, 'z': this.targetZ } or something. And the second one flips x and z add adds a negative sign, so I'd have to read the code to figure out what's going on and I'd rather not do that. :)

In D1037#40924, @temple wrote:

My point is the first one isn't a position, I'd have to create { 'x': this.targetX, 'z': this.targetZ } or something.

atan isn't the only abstraction possible there. Both files look like they would be better off and use vectors from the get go.

And the second one flips x and z add adds a negative sign

That sounds a lot like what perpendicular() does (exactly the reason why we should use vector notation instead of combining equations IMO).

so I'd have to read the code to figure out what's going on and I'd rather not do that. :)

If one can prove that the code exactly works as before, whatever it did, one can refactor things without having found its deeper meaning.
But you are right that often (eh, always) one thing leads to another.
Since I'm already cought up in #4845 I might give it a shot.

In D1037#40938, @elexis wrote:

In D1037#40924, @temple wrote:

And the second one flips x and z add adds a negative sign

That sounds a lot like what perpendicular() does (exactly the reason why we should use vector notation instead of combining equations IMO).

That's probably right, since walls face "out".

Just to be explicit about rmgen, it uses the normal convention for angles (east = 0, north = pi/2):

/**
 * Returns the angle of the vector between point 1 and point 2.
 * The angle is counterclockwise from the positive x axis.
 */
function getAngle(x1, z1, x2, z2)
{
	return Math.atan2(z2 - z1, x2 - x1);
}

Having thought a bit, I guess I'm not opposed to having the 2D version with a comment about the weird convention (north = 0, east = pi/2), although it won't be used for anything at the moment, unless possibly if walls or flying are rewritten. (I'm assuming we'd keep the 3D version too, so we wouldn't have to convert positions to 2D just to perform this calculation. But maybe you disagree?)

I don't think angleBetween is a great name for vectors, because I picture the angle between the two vectors as seen from the origin, e.g. 90 degrees for (0,1) to (1,0), whereas angleTo leads me to think of the vectors as positions, i.e. to figure out the angle of the vector representing the difference of the two positions. (I suppose we could have a function for the angle or argument of a single vector, similar to how we have length for one vector and distanceTo for two. Maybe the 3D version could be horizAngleTo, like horizDistanceTo?)

About the rmgen code, we have this stuff from rP11430 which is the commit I wasn't able to judge yet. The content sounds exactly like what you describe.

Map.prototype.exportEntityList = function()
{
	// Change rotation from simple 2d to 3d befor giving to engine
	for (let obj of this.objects)
		obj.rotation.y = Math.PI / 2 - obj.rotation.y;

All of these places eventually should use one coordinate system so that noone has to wonder wtf is going on and that comments about weird conventions don't have to exist or understood.
We should aim at having only one angle Vector2D function ideally.

Should likely avoid adding an unused function.
Perhaps we could add a function to convert the rotation from one system to another instead of adding two vector functions.
But I still don't know which part needs one angle function and which other code needs the other one?

If it's only rmgen, screw that.
Within the simulation I expect everything to require the same atan.
Is it the gui wall placement code? Maybe placementSupport.position could use a Vector2D, but it can also be ignored since the entire rattail would have to be checked, equally to the other two files you mentioned.

so we wouldn't have to convert positions to 2D just to perform this calculation

In the cases I saw so far the code only used x/z where the angle was computed, so the change would be calling GetPosition2D and renaming Z to Y instead of a conversion from 3D to 2D. Depends on the places touched.

horizAngleTo

Sounds good, but only if we can't avoid the 3D one easily.

In D1037#40986, @elexis wrote:

All of these places eventually should use one coordinate system so that noone has to wonder wtf is going on and that comments about weird conventions don't have to exist or understood.

Right.

But I still don't know which part needs one angle function and which other code needs the other one?
If it's only rmgen, screw that.

I think everything uses the north = 0 style except for rmgen. My GUI talk was in reference to your hypothetical "(for instance computing angles between two GUI objects might be a use case)", where I was thinking people in the future might get confused if they were expecting east = 0.

So implement one Vector2D function, leave out rmgen and it'll be good.

I appreciate how much effort you put into writing patches. Could have likely gotten away with less typing and compiling effort by asking if the footprint c++ code should be changed.

Changing this function to 2D seems like a regression as there could easily be a future user of a 3D function.
I had the GetPosition2D call in mind. If we need to change the C++ part inevitably, then we could still create a 2D footprint getter variant that just calls the 3D getter and converts the result to a 2D vector.
But why not use a Vector2D.from3D in JS?
Would a JS conversion be too slow?

In D1037#41102, @elexis wrote:

But why not use a Vector2D.from3D in JS?
Would a JS conversion be too slow?

I guess I don't understand why you insist on not having a horizAngleTo function for 3D positions.
I don't know how much slower Vector2D.from3D would be, but it's really ugly anyway, right?
If spawn points must be 3D, then of the four places where we want to use AngleTo, only one of them (UnitAI) naturally uses two 2D positions.
So to me it seems obvious to create a horizAngleTo.

I didn't notice the two Footprint occurrences until this diff.
The other vectors are initialized from cmpPosition and only use the 2D values, so appear replaceable with GetPosition2D without increasing the net amount of code and slightly reducing the performance (though increasing the diff).

But I do agree that horizAngleTo without comments about ingame usage would be preferable over changing C++ or converting the Footprint vectors in JS.

I didn't realize that calculating the y-coordinate takes a little bit of work (since we only store the relative height) and I don't know how that compares in performance to creating a new JS object or if that matters much.

So for the spawning point cases, which do you prefer?

cmpNewPosition.SetYRotation(cmpPosition.GetPosition().horizAngleTo(pos));
cmpNewPosition.SetYRotation(cmpPosition.GetPosition2D().angleTo(Vector2D.from3D(pos)));

(I don't care at this point. :) )

In D1037#41132, @temple wrote:

I didn't realize that calculating the y-coordinate takes a little bit of work (since we only store the relative height) and I don't know how that compares in performance to creating a new JS object or if that matters much.

So for the spawning point cases, which do you prefer?

cmpNewPosition.SetYRotation(cmpPosition.GetPosition().horizAngleTo(pos));
cmpNewPosition.SetYRotation(cmpPosition.GetPosition2D().angleTo(Vector2D.from3D(pos)));

With computing the Y coordinate you must mean the GetHeightFixed() call in GetPosition() of CCmpPosition.cpp.
Running a piece of code in a loop and measuring the time with Date.now() in milliseconds reveals what variant is faster.
Since ungarrisoning and training units occurs rarely and with only few units and since I expect both variants to only consume only a handful of microseconds per entity, it ought not to matter performance-wise.
If I understand correctly you only need the 3D angle variant in the other places, so we could aim for a shorter diff (even though the other function sounds like it would be required eventually too the more Vector usage we add).

A quick performance comparison should still be done, considering that my expectations of performance failed horribly recently (rP20351).

With a million loops I get numbers like this, where angleToo compares the 2D and 3D vectors directly.

cmpPosition.GetPosition().horizAngleTo(pos);
took = 1938 milliseconds
cmpPosition.GetPosition2D().angleTo(Vector2D.from3D(pos));
took = 1414 milliseconds
cmpPosition.GetPosition2D().angleToo(pos);
took = 1384 milliseconds

Less than a couple microseconds per unit, seems not a big deal. So I'll use horizAngleTo for the two spawns because that looks nicer.
I'll use 3D for input and 2D for UnitAI. It seems Wall will use 2D, but not sure about UnitMotionFlying.

About the prior patch (History id 4224) with the CCmpFootprint change:

I should have read it more closely the first time, because I didn't notice it returns a 3D vector where the Y coordinate is set to zero, which of course is a blatant lie!

According to my search, the Footprint component is the only C++ components doing such a thing and I think it's misleading enough to consider that a separate bug:

ICmpPlayer.cpp // These are mostly unused and should be deleted last time I checked.
46: virtual CFixedVector3D GetStartingCameraPos() 
51: virtual CFixedVector3D GetStartingCameraRot() 

ICmpPosition.cpp
25: DEFINE_INTERFACE_METHOD_2("SetTurretParent", void, ICmpPosition, SetTurretParent, entity_id_t, CFixedVector3D) 
41: DEFINE_INTERFACE_METHOD_CONST_0("GetPosition", CFixedVector3D, ICmpPosition, GetPosition) 
43: DEFINE_INTERFACE_METHOD_CONST_0("GetPreviousPosition", CFixedVector3D, ICmpPosition, GetPreviousPosition) 
48: DEFINE_INTERFACE_METHOD_CONST_0("GetRotation", CFixedVector3D, ICmpPosition, GetRotation) 

ICmpProjectileManager.cpp
25: DEFINE_INTERFACE_METHOD_4("LaunchProjectileAtPoint", uint32_t, ICmpProjectileManager, LaunchProjectileAtPoint, entity_id_t, CFixedVector3D, fixed, fixed) 

ICmpRangeManager.cpp
55: DEFINE_INTERFACE_METHOD_CONST_5("GetElevationAdaptedRange", entity_pos_t, ICmpRangeManager, GetElevationAdaptedRange, CFixedVector3D, CFixedVector3D, entity_pos_t, entity_pos_t, entity_pos_t) 

ICmpSoundManager.cpp
26: DEFINE_INTERFACE_METHOD_2("PlaySoundGroupAtPosition", void, ICmpSoundManager, PlaySoundGroupAtPosition, std::wstring, CFixedVector3D) 

ICmpTerrain.cpp
26: DEFINE_INTERFACE_METHOD_CONST_2("CalcNormal", CFixedVector3D, ICmpTerrain, CalcNormal, entity_pos_t, entity_pos_t)

So the question for Footprint that remains is whether it should return the real Y coordinate or return a 2D vector.
It's still not unlikely for multiple use case to occur where we need the Y axis of the Footprint location.
The component doesn't load the height yet, so we could implement a 3D getter that calls the 2D function and loads the height from some other component then.
But there might just as well be a patch like D1040 that makes the Footprint component load the height and could thus fix the blatant lie and coincidentally keep your currently proposed diff ideal.

(If we would change the return value to 2D, this should still not be done in this revision proposal because it at goes too far beyond the scope of the commit for the proposed feature.
In fact the Vector addition and cleanup of existing code can be committed separately as well (no merge conflicts, only different hunks of the same patch). I can split the hunks when committing, no need to reup.)

Thanks for your performance test as well. It is relieving to have the certainty.

The location of the new functions is ideal.
The order of arguments of the prior and new code match and the order of arguments in the vector function is the more reasonable of the two choices.
Code still works as expected when testing.
Thanks for the feature idea, the implementation that fixes the TODO and the collaborative assessment!