globals should be g_Foo (input.js has it wrong, but better add it correctly than having to change it later)

a sentence should end with a period

(I actually meant all 3 could be const, so only the upper one is a var)

We should find a unique catchy name like freehand selection, freehand formation, or input line and then make it g_Name_Property, so tht it is easily distinguished from other globals and so that the reader knows the connotation immediately even if there is no danger of confusion

to make code clearer, these could be const

Why not make it 2 then?

Minimum

periods

(Why take the square but not the other function from that commit?)

Could use some bipartite weighted matching algorithm.
For example the Hungarian algorithm (https://en.wikipedia.org/wiki/Hungarian_algorithm). Though I don't know if that will be fast enough...

no

should become a Vector2D, can use Vector2D.sub and distanceTo then to simplify the code a bit

Sure dragStart isn't better off as a new Vector2D if we're already creating a new object?

we do not put spaces after [ and before ]

compare the squares for performance, also sqrt takes only one argument,
we should check against maxDragDelta right?

(Always use let instead of var so that the reader only has to consider the scope that the variable is defined in. Also the variable is used only once so can be inlined.)

-1 tab

Why not:

if (action)
   return doAction...;

There is already a break on L1085

why not the euclidic distance?
like this it triggers at different limits depending on the direction...

MouseMove

I would have said target.distanceToSquared(FreehandSelection_InputLine[g_FreehandSelection_InputLine.length - 1]), but this one might be faster.
But then why does it actually square the number if we don't use euclidian distance anymore and just not use Math.abs?
This is only executed when sending a command and this only saves some microseconds I guess?

wasn't the argument to change these lines in a previous iteration that it is a different distance for different directions?

Vector2D.from3D

language

also could compare the squares for performance

also taking the square helps for performance (one could also store the resolutionSquared for even better performance)

spaces around -

let

language

could be

let p0 = inputLine[0];
let entityDistribution = [p0];

maybe freeDist

min => minimum

could use p1

That magic number 1 should become a global, so that the meaning is expressed in the variable name and it can be changed without reading the code.

!...length

missing points and is that comma correct?

technically check unneeded, I am fine with keeping them (yellers may yell)

(unneeded commment)

Could be merged like this?

let p = ...
if (...)
    g_InputLine.push(p);

(Besides the braces of that if not being needed, the if itself is not needed / implied by the loop either, right?

inline

should work with for...of too

(comment implies there could be more that 1 point missing, but code doesn't handle that)

missing or too many
Isn't that one too many at most?

many points

If one wants to take into account multiple points missing, one could
entityDistribution = entityDistribution.concat(new Array(selection.length - entityDistribution.length).fill(inputLine[inputLine.length - 1])

(check theoretically unneeded that way)

else if
(Theoretically check isn't needed)

vector1.distanceTo(vector2)

inline that in L1186

(could be slice aswel, no strong opinion)

I'd probably set this a lot higher.

use a cap

is there a real need for the minlength?

spaces around - (same below)

unneeded parentheses, && binds stronge than || like * and +, in doubt lookup "operator precedence"

inline

we doesn't seem to need the index, so we should use for ...of
Also perhaps i=> pos

let reworkedLine = [inputLine[0]]

reworkedLine => entityDistribution

distanceBetweenEnts?

There are some edgy cases here still, f.e. allign the units on a line, and send them to a line orthagonal on the first, with one of the endpoints on an endpoint of the original line (So we turn the line 90deg), I would expect the unit on the endpoint of both lines to stay where it is, and swing the line over, However (lets assume that unit is the first on the first corner, argument is symmetrical) then firsttofirst < firsttoLast, but neither of the first conditions holds.
Maybe it would work to check firstToFirst+lasttoLast>firstTolast+lasttofirst (as that would minimise the straight line distance in such a case)
Testing with that looks ok to me

If you accidentally select a cc and drag the mouse when setting a rally point, you'll get:

ERROR: JavaScript error: uncaught exception: out of memory

Because if selection.length = 0, shouldDist is negative and the while loop on L1244 doesn't end.
Should probably be careful about dividing by zero too.

I think you want to use the reverse() method

no space needed after -

return action && doAction

could be a forEach

unneeded braces

Engine.HotkeyIsPressed("session.queue")

This will spam the replay files. Not sure if there is a performance overhead attached too.
It seems cleaner to extend Commands.js by a new command type and then do the unit commanding in there, would you agree @bb, @Imarok?
The computation of destination positions should likely remain in here, so that we pass as few information as possible over (also I guess screen coordinates only make sense in this file).

shouldn't we multiply with shouldDist?

(Perhaps its nicer to use Vector functions here to avoid the duplication of the computation for the second axis. Performance likely no issue as this only occurs on button up. In fact it looks like x0 and z0 are exactly the coordinates of that vector 3 lines above. Also some of the vectors used in here (x1-z1) could be computed above the while loop)

what if there are two missing points?

+1 tab

(comment redundant with the code)

That vector is created twice. Also doesn't the entity state have that vector already?

Some of this code looks duplicated. Could it be moved to a helper function?

This looks broken (as in missing property names and spaces)

== null is also true if == 0 if I'm not mistaken (see "falsey"). If you want that you can just use !minDist, otherwise === null

(probably better from a performance point of view to use the pythagorean distance function from Math rather than creating an object that is used only once)

(looks like it could use for (let ent in selection), or for...of?)

Vector2D.sub(vector1, vector2) seemed nicer than clone.sub. normalize() instead of division by dist

if reworkedLine.length> selection.length we will fail here, maybe change it around and let every unit go to the closest free spot on the line

min => minimum

instead of passing reworkedLine and i it could be reworkedLine[i]

for of

vec1.distanceTo(vec2)