I thought distanceTo needs vectors, not ships, but I was wrong

[Milo]

In Resources\ai\oolite-assassinAI.js, the following line is passing this.ship to distanceTo, instead of this.ship.position:

Code: Select all

	if (system.mainStation && system.mainStation.position.distanceTo(this.ship) < 25000 && Math.random() < 0.5)

The return value of distanceTo when you pass in a non-vector is "NO" (defined in Objective C as boolean zero). So, the effect here is that assassin ships spawned anywhere in the system have a 50% chance to head to the station aegis (because zero is always less than 25000), which is not what was intended.

There are a few more instances of this error if you search the full repository: https://github.com/OoliteProject/oolite ... distanceTo

1. All (7) of the distanceTo checks in oolite-tutorial.js. Here's a link to a fixed copy of the script: https://pastebin.com/3NUKx5wi
2. One check in oolite-constrictor.js (but it seems that nothing ever calls the _checkDistance function in there?)
3. Many in oolite-priorityai.js. Also take note of this.distance() calls which wrap distanceTo. Here's a link to a copy of oolite-priorityai.js (taken from the 1.89.0.200612 nightly, which is one behind but it hasn't changed) where I went through all 105 matches for "distance" and made corrections where needed (almost all of them): https://pastebin.com/QzPMdMRL

Since there were so many errors in the AI script, I will be very interested to see the "intended" behavior of the AI now...

The implementation of distanceTo is:

Code: Select all

// distanceTo(v : vectorExpression) : Number
static JSBool VectorDistanceTo(JSContext *context, uintN argc, jsval *vp)
{
	OOJS_PROFILE_ENTER
	
	HPVector					thisv, thatv;
	double						result;
	
	if (EXPECT_NOT(!GetThisVector(context, OOJS_THIS, &thisv, @"distanceTo"))) return NO;
	if (EXPECT_NOT(!VectorFromArgumentList(context, @"Vector3D", @"distanceTo", argc, OOJS_ARGV, &thatv, NULL)))  return NO;
	
	result = HPdistance(thisv, thatv);
	
	OOJS_RETURN_DOUBLE(result);
	
	OOJS_PROFILE_EXIT
}

It might be a good idea to actually log a warning when a non-vector is provided for either argument. Note that calling this.ship.distanceTo(that.ship.position) is also incorrect because there's no position vector before distanceTo.

I've also updated the wiki page with a caution.

[another_commander]

Although a position vector is required for the entity to which the distanceTo is applied, the argument of the distanceTo function can be a vector, an array or an entity. In the last case it uses the entity's position as argument. Check out OOJSVector.m, JSObjectGetVector function; there is a comment about this too at line 380.

So there is no bug here, all scripts and references of distanceTo in the core scripts are correct.

[Milo]

Ah, I see. I should've looked deeper. Glad to know this was a false alarm. Wiki has been corrected.

Edited to add: However, it should be slightly more efficient to pass in vectors directly since that case is handled first, followed by the array check and then the entity check. I don't know how much impact it would have but perhaps it is worth incorporating the changes I made?

[another_commander]

However, it should be slightly more efficient to pass in vectors directly since that case is handled first, followed by the array check and then the entity check. I don't know how much impact it would have but perhaps it is worth incorporating the changes I made?

As surprising as this may sound, the way we have it right now in the core scripts is the fastest of the two options. Profiling is our friend here. Look how the expression PS.position.distanceTo(PS.target.position) profiles in comparison to the equivalent one with the .position part taken out, when I launch from Lave and immediately stop and target the buoy:

Code: Select all

> a=function(){PS.position.distanceTo(PS.target.position)}
function () {
    PS.position.distanceTo(PS.target.position);
}
> console.profile(a)
Total time: 0.115 ms
JavaScript: 0.036 ms, native: 0.072 ms
Counted towards limit: 0.0759943 ms, excluded: 0.0390057 ms
Profiler overhead: 0.052 ms
                                                        NAME  T  COUNT    TOTAL     SELF  TOTAL%   SELF%  SELFMAX
                               (<console input>) <anonymous>  J      1     0.11     0.04    93.9    31.3     0.04
                                           EntityGetProperty  N      2     0.04     0.02    38.3    20.9     0.02
                                           HPVectorToJSValue  N      2     0.01     0.01    13.0    10.4     0.01
                                             ShipGetProperty  N      1     0.01     0.01    10.4     8.7     0.01
                                            VectorDistanceTo  N      1     0.02     0.01    13.9     8.7     0.01
                                         OOJSEntityGetEntity  N      2     0.00     0.00     4.3     4.3     0.00
                       VectorFromArgumentListNoErrorInternal  N      1     0.00     0.00     4.3     3.5     0.00
                                        JSVectorWithHPVector  N      2     0.00     0.00     2.6     2.6     0.00
                                         JSShipGetShipEntity  N      1     0.00     0.00     1.7     1.7     0.00
                                           JSObjectGetVector  N      2     0.00     0.00     1.7     1.7     0.00
> b=function(){PS.position.distanceTo(PS.target)}
function () {
    PS.position.distanceTo(PS.target);
}
> console.profile(b)
Total time: 0.097 ms
JavaScript: 0.029 ms, native: 0.062 ms
Counted towards limit: 0.0699954 ms, excluded: 0.0270046 ms
Profiler overhead: 0.041 ms
                                                        NAME  T  COUNT    TOTAL     SELF  TOTAL%   SELF%  SELFMAX
                               (<console input>) <anonymous>  J      1     0.09     0.03    93.8    29.9     0.03
                                        JSVectorWithHPVector  N      1     0.02     0.02    16.5    16.5     0.02
                                           EntityGetProperty  N      1     0.04     0.01    39.2    14.4     0.01
                                             ShipGetProperty  N      1     0.01     0.01    10.3     9.3     0.01
                                            VectorDistanceTo  N      1     0.01     0.01    14.4     7.2     0.01
                                           HPVectorToJSValue  N      1     0.02     0.00    21.6     5.2     0.00
                       VectorFromArgumentListNoErrorInternal  N      1     0.01     0.00     6.2     4.1     0.00
                                         OOJSEntityGetEntity  N      1     0.00     0.00     3.1     3.1     0.00
                                           JSObjectGetVector  N      2     0.00     0.00     3.1     3.1     0.00
                                         JSShipGetShipEntity  N      1     0.00     0.00     1.0     1.0     0.000

Although the VectorDistanceTo native (core) function is essentially the same execution-time wise in both versions, the version that has the .position omitted is overall slightly faster. This is a repeatable observation and was noted also in this wiki page a few years ago.

Edit: For an explanation of what these tables contain, refer to this post.

[dybal]

However, it should be slightly more efficient to pass in vectors directly since that case is handled first, followed by the array check and then the entity check. I don't know how much impact it would have but perhaps it is worth incorporating the changes I made?

As surprising as this may sound, the way we have it right now in the core scripts is the fastest of the two options. Profiling is our friend here. Look how the expression PS.position.distanceTo(PS.target.position) profiles in comparison to the equivalent one with the .position part taken out, when I launch from Lave and immediately stop and target the buoy:

Code: Select all

> a=function(){PS.position.distanceTo(PS.target.position)}
function () {
    PS.position.distanceTo(PS.target.position);
}
> console.profile(a)
Total time: 0.115 ms
JavaScript: 0.036 ms, native: 0.072 ms
Counted towards limit: 0.0759943 ms, excluded: 0.0390057 ms
Profiler overhead: 0.052 ms
                                                        NAME  T  COUNT    TOTAL     SELF  TOTAL%   SELF%  SELFMAX
                               (<console input>) <anonymous>  J      1     0.11     0.04    93.9    31.3     0.04
                                           EntityGetProperty  N      2     0.04     0.02    38.3    20.9     0.02
                                           HPVectorToJSValue  N      2     0.01     0.01    13.0    10.4     0.01
                                             ShipGetProperty  N      1     0.01     0.01    10.4     8.7     0.01
                                            VectorDistanceTo  N      1     0.02     0.01    13.9     8.7     0.01
                                         OOJSEntityGetEntity  N      2     0.00     0.00     4.3     4.3     0.00
                       VectorFromArgumentListNoErrorInternal  N      1     0.00     0.00     4.3     3.5     0.00
                                        JSVectorWithHPVector  N      2     0.00     0.00     2.6     2.6     0.00
                                         JSShipGetShipEntity  N      1     0.00     0.00     1.7     1.7     0.00
                                           JSObjectGetVector  N      2     0.00     0.00     1.7     1.7     0.00
> b=function(){PS.position.distanceTo(PS.target)}
function () {
    PS.position.distanceTo(PS.target);
}
> console.profile(b)
Total time: 0.097 ms
JavaScript: 0.029 ms, native: 0.062 ms
Counted towards limit: 0.0699954 ms, excluded: 0.0270046 ms
Profiler overhead: 0.041 ms
                                                        NAME  T  COUNT    TOTAL     SELF  TOTAL%   SELF%  SELFMAX
                               (<console input>) <anonymous>  J      1     0.09     0.03    93.8    29.9     0.03
                                        JSVectorWithHPVector  N      1     0.02     0.02    16.5    16.5     0.02
                                           EntityGetProperty  N      1     0.04     0.01    39.2    14.4     0.01
                                             ShipGetProperty  N      1     0.01     0.01    10.3     9.3     0.01
                                            VectorDistanceTo  N      1     0.01     0.01    14.4     7.2     0.01
                                           HPVectorToJSValue  N      1     0.02     0.00    21.6     5.2     0.00
                       VectorFromArgumentListNoErrorInternal  N      1     0.01     0.00     6.2     4.1     0.00
                                         OOJSEntityGetEntity  N      1     0.00     0.00     3.1     3.1     0.00
                                           JSObjectGetVector  N      2     0.00     0.00     3.1     3.1     0.00
                                         JSShipGetShipEntity  N      1     0.00     0.00     1.0     1.0     0.000

Although the VectorDistanceTo native (core) function is essentially the same execution-time wise in both versions, the version that has the .position omitted is overall slightly faster. This is a repeatable observation and was noted also in this wiki page a few years ago.

Edit: For an explanation of what these tables contain, refer to this post.

Would that be true (passing an entity instead of its position) for other methods that expect a vector as argument (like Vector.subtract())?

[Milo]

It appears that Vector3D.subtract uses the same evaluation method as distanceTo, so it should be possible to pass it an entity instead of a vector. And I suspect it would have a similar benefit, as it seems the cost of dereferencing a property in JS exceeds the cost of a couple of extra checks in Obj-C.

Incidentally, I expect that switching the order of the array vs. entity check in the core JSObjectGetVector function in OOJSVector.m would have a slight benefit for performance, as I think the passing of entity is more common (certainly this is true in OXPs) than the passing of array-type arguments.