Strict mode warnings

[Milo]

Code: Select all

Warning (strict mode): reference to undefined property ship.missiles[i].primaryRole
    Active script: oolite-populator 1.89
    oolite-populator.js, line 2306:
    			if (ship.missiles[i].primaryRole == "EQ_MISSILE" && chance < Math.random())

I haven't looked into this one.

Code: Select all

Warning (strict mode): reference to undefined property market[commodity].marketMaskPrice
    Active script: Oolite Trader AI 1.89
    oolite-priorityai.js, line 2543:
    				var adjust = market[commodity].marketEcoAdjustPrice * multiplier * quantity / market[commodity].marketMaskPrice;

Warning (strict mode): reference to undefined property market[commodity].marketEcoAdjustPrice
    Active script: Oolite Trader AI 1.89
    oolite-priorityai.js, line 2543:
    				var adjust = market[commodity].marketEcoAdjustPrice * multiplier * quantity / market[commodity].marketMaskPrice;

For the market warnings, I at first thought of an unknown commodity name (which would also be a problem here), but when I scanned in-system ships I found no commodities in their cargoList that didn't have an entry at the main station. However, none of the system.mainStation.market entries had a marketEcoAdjustPrice or marketMaskPrice property.

I think it could be fixed by enclosing lines 2543 and 2544 in an if block like this:

Code: Select all

				if (market[commodity].marketEcoAdjustPrice && market[commodity].marketMaskPrice) // defined and non-zero (for division)
				{
					var adjust = market[commodity].marketEcoAdjustPrice * multiplier * quantity / market[commodity].marketMaskPrice;
					profit += adjust;
				}

Next warning:

Code: Select all

Warning (strict mode): reference to undefined property this.__ltcache.oolite_homeStation
    Active script: YAH patrol AI 4.5.1
    oolite-priorityai.js, line 903:
    	if (this.__ltcache.oolite_homeStation === null)

Suggested revision beginning at line 901 of oolite-priorityai.js:

Code: Select all

PriorityAIController.prototype.homeStation = function() 
{
	if (this.__ltcache.oolite_homeStation !== undefined)
	{
		if (this.__ltcache.oolite_homeStation === null)
		{
			return null;
		}
		if (this.__ltcache.oolite_homeStation.isValid)
		{
			return this.__ltcache.oolite_homeStation;
		}
	}
	// home station has not been assigned yet (property undefined)
	// home station might be the owner of the ship, or might just
	// be a group member
	if (this.ship.owner && this.ship.owner.isStation && this.friendlyStation(this.ship.owner))
	{
		this.__ltcache.oolite_homeStation = this.ship.owner;
		return this.ship.owner;
	}
	if (this.ship.group)
	{
		var gs = this.ship.group.ships;
		for (var i = gs.length-1 ; i >= 0  ; i--)
		{
			if (gs[i] != this.ship && gs[i].isStation && this.friendlyStation(gs[i]))
			{
				this.__ltcache.oolite_homeStation = gs[i];
				return gs[i];
			}
		}
	}
	this.__ltcache.oolite_homeStation = null;
	return null;
}

Next pair of warnings (note, my line numbers may be slightly different due to changes mentioned above):

Code: Select all

Warning (strict mode): reference to undefined property this.ship.group.leader.AIScript.oolite_intership.cargodemanded
    Active script: Oolite Pirate AI 1.89
    oolite-priorityai.js, line 2039:
    			if (this.ship.group.leader && this.ship.group.leader.AIScript.oolite_intership && this.ship.group.leader.AIScript.oolite_intership.cargodemanded > 0)

Warning (strict mode): reference to undefined property this.ship.group.ships[0].AIScript.oolite_intership.cargodemanded
    Active script: Oolite Pirate AI 1.89
    oolite-priorityai.js, line 2048:
    			else if (this.ship.group.ships[0].AIScript.oolite_intership && this.ship.group.ships[0].AIScript.oolite_intership.cargodemanded > 0)

One way to "fix" this (the code works, but strict mode dislikes it) without too much added code is to provide a default value for the comparison, like this:

if (this.ship.group.leader && this.ship.group.leader.AIScript.oolite_intership && (this.ship.group.leader.AIScript.oolite_intership.cargodemanded || 0) > 0)

With this, if this.ship.group.leader.AIScript.oolite_intership.cargodemanded is undefined, the || 0 replaces it with zero. The result of 0 > 0 is false. undefined > 0 also is false, but also triggers a warning.

[Milo]

Another one (line number may be slightly off due to my local edits):

Code: Select all

Exception: TypeError: group.leader.group is null
    Active script: Oolite Escort AI 1.89
    oolite-priorityai.js, line 1105:
    			gs = group.leader.group.ships;

[cag]

...
<long rambling post>
...

TL;DR I fell down a deep rabbit hole and applied strict mode and various optimizations to all the scripts in the Resources\Scripts folder that comes with the core. I left the AI scripts alone, as I would need more I before messing with A's

So before you fall into that same rabbit hole, try substituting these and see if the problems persists. The core scripts have changed little in the past (almost 3 f@#! years) and I've kept them current. I've been using them since then but I don't have your knack of blowing up code!

https://www.dropbox.com/s/cudwuq8d56epc ... s.zip?dl=0

[Milo]

Thanks. I'm confused, though, why move "use strict"; inside the scope of the this.activated function in oolite-cloaking-device-equipment.js and why name the function with 'this.activated = function activated()' instead of just 'this.activated = function()'? In the core version of the file, it has "use strict"; at the top outside the function and omits naming the function.

I don't understand the purpose of the changes in oolite-cloaking-device-mission.js either. Why put everything in a closure function? Why pull out the definition of the callback function used in the populator? What is the meaning of creating a variable pointing to the function whose scope contains it (var 'that'), and what is "var setPopulator = (that.setPopulator = that.setPopulator || system.setPopulator);" doing?

At line 1190 of oolite-populator.js (your version), the property is missing an 's' at the end:

if (random() < repop_freq_In.thargoidStrikes)

This was very recently fixed in the core.

[cag]

why move "use strict"; inside the scope of the this.activated function

To satisfy JSLint, which kept complaing 'Use the function form of "use strict". For scripts with more than one function, this would become tedious, so we wrap it all in a function.

You don't need to do it for every function. Just wrap your code in a function. Please also note that the jshint and the global settings will vary from script to script - sometimes you don't need them at all.

Code: Select all

/* jshint <your options> */
/* global <your global vars> */
(function(){
"use strict";
<your code>

}).call(this);

I never did find out if it matters much to Oolite's JS interpreter. It does no harm and I don't have to sift through all of those errors to find the ones that matter.

why name the function with 'this.activated = function activated()' instead of just 'this.activated = function()'

I got real annoyed of messages saying '<anonymous function>'. This way, I get the name I've given to the function . The real benefit comes when you're profiling: it gives line numbers but with a named function, it's much easier to interpret. The name between 'function' and '()' is just that, a name - can be anything you want; it has no other effect than to make messages less annoying.

In this particulr case, it probably doesn't matter. I just got in the habit of naming all my functions.

I don't understand the purpose of the changes in oolite-cloaking-device-mission.js either. Why put everything in a closure function?

Cleaner code, in that "use strict" appears only once. I've seen some that wrap the entire script, including this.name on the 1st line. We're not a very consistent bunch.

Why pull out the definition of the callback function used in the populator? What is the meaning of creating a variable pointing to the function whose scope contains it (var 'that'), and what is "var setPopulator = (that.setPopulator = that.setPopulator || system.setPopulator);" doing?

I'll answer these in reverse order. The third was all Day's fault
viewtopic.php?p=257444#p257444
We're creating a property for the function called 'setPopulator'. The first time the script is invoked, it fetches a reference to the 'system.setPopulator' function and saves it in this new property. All subsequent calls do not have to fetch it, as it's stored locally. Why you ask? Speed. Maps (aka dictionaries) are much faster than arrays to search but they still take time. worldScripts is the worst as it's HUGE. When profiling timers & frame callbacks, these property 'gets' can account for much of the execution time. And it's a waste, as the reference is static - the function is always at that same reference.

The name of the var 'that' is nothing special, just thematically similar to this. Having named the function 'systemWillPopulate', I could have just used that instead. The reason we use a second variable is so we can cut and paste these reference caches from function to function. All we have to change is the value assigned to 'that'.

As for pulling out the callback function, besides being good practice and easier to read, it was a side effect of the next line

Code: Select all

var ents = (that.ents = that.ents || {});

where 'ents' is another function property containing an object which we reuse. In the original, every call to that function would create a new object which then gets discarded. Another pet peeve of mine is those little freezes that seem to happen at the worst possible moment. That's JS performing garbage collection. JS is much easier to code in than, say, C, where you're responsible for memory management. The price we pay is garbage collection. This is performed whenever you launch or exit witch space, when you'll never notice it. But with more oxp's loaded, some will be generating garbage, leading to these mini-strokes oolite has.

All of this probably has little if any effect here, as this function is rarely called. The main purpose of doing this consistently across all the scripts was to have a ready example for new oxp authors to reference.


FYI, I just installed the latest nightly and there were a couple tweaks in oolite-populator.js, so you'll have to download that zip file again, same line.

https://www.dropbox.com/s/cudwuq8d56ep ... .zip?dl=0

=====================================================================

At line 1190 of oolite-populator.js (your version), the property is missing an 's' at the end:

if (random() < repop_freq_In.thargoidStrikes)

This was very recently fixed in the core.

That was fast. Here I am about to post this reply to tell you the same thing. That was one of two changes to that script.

[Day]

[Milo]

cag, with your scripts, I still get this error:

Code: Select all

Exception: TypeError: leader.group is null
    Active script: Oolite Police AI 1.89
    oolite-priorityai.js, line 1189:
    			gs = leader.group.ships;

[cag]

Thanks for the tip. New version uploaded, same link

https://www.dropbox.com/s/cudwuq8d56ep ... .zip?dl=0

[Milo]

Another one:

Code: Select all

Exception: TypeError: this.ship.group is null
    Active script: Oolite Escort AI 1.89
    oolite-priorityai.js, line 6100:
    	if (ally == this.ship.group.leader)

[cag]

You're starting to freak me out, dude. While that line 6100 was from the core distribution, the same error is in mine too. That's been there for YEARS! How are you finding these?

New version uploaded, same link

https://www.dropbox.com/s/cudwuq8d56ep ... .zip?dl=0

[Milo]

I'm cursed, and/or I have a lot of OXPs that populate my systems heavily and expose edge cases, and/or I'm running with the debug console and pedantic mode, so I see all the errors, and/or I'm cursed.

[Milo]

cag, I'm now using your revised scripts. Debug console reported:

Code: Select all

will launch from Torus Station 9291 flying Kirin Sport 32256
Exception: TypeError: hs[i] is null
    Active script: telescope 2.0
    oolite-populator.js, line 1667:
    			hs[i].group = group;
3 big trader(s) added to the Onrira system.

Latest.log with more detail (looks like it's the same issue with a failed hunter launch that I reported earlier with the core populator):

Code: Select all

13:56:24.167 [LogEvents] GlobalLog: will launch from Torus Station 9291 flying Kirin Sport 32256
13:56:24.357 [station.launchShip.failed] -[StationEntity fitsInDock:andLogNoFit:]: Cancelled launch for a Orca Class Destroyer with role hunter, as it is too large for the docking port of the <StationEntity 0x21ba9210>{"Torus Station" "Torus Station" position: (-70100.6, -111209, 741933) scanClass: CLASS_STATION status: STATUS_ACTIVE}.
13:56:24.882 [script.javaScript.exception.unexpectedType] ReportJSError: ***** JavaScript exception (telescope 2.0): TypeError: hs[i] is null
13:56:24.883 [script.javaScript.exception.unexpectedType] ReportJSError:       Resources/Scripts/oolite-populator.js, line 1667.
13:56:24.885 [script.debug.note.addShips] -[PlayerEntity(Scripting) addShipsWithinRadius:]: DEBUG: Going to add 1 ship(s) with role 'bigTrader' within 36060.00 radius about point (0.000, 0.000, 0.235) using system pwu
13:56:24.940 [script.debug.note.addShips] -[PlayerEntity(Scripting) addShipsWithinRadius:]: DEBUG: Going to add 1 ship(s) with role 'bigTrader' within 36060.00 radius about point (0.000, 0.000, 0.399) using system pwu
13:56:25.103 [script.debug.note.addShips] -[PlayerEntity(Scripting) addShipsWithinRadius:]: DEBUG: Going to add 1 ship(s) with role 'bigTrader' within 36060.00 radius about point (0.000, 0.000, 0.568) using system pwu
13:56:25.176 [bigShips_populator] GlobalLog: 3 big trader(s) added to the Onrira system.

[cag]

Ok, that's a problem in the core's scripts and mine as well. Thargoid's Aquatics is over 6 years old! Great work on ferreting out this one.

I added a test in _addShips in case groups w/ a different role get stuck in the docking port.

New version uploaded, same link

https://www.dropbox.com/s/cudwuq8d56ep ... .zip?dl=0

[Milo]

Here's another error in your revised scripts, cag:

Code: Select all

Exception: TypeError: targ_posn is undefined
    Active script: Oolite Police AI 1.89
    oolite-priorityai.js, line 1178:
    			if (ship != this.ship && targ_posn.distanceTo(ship) < scan_range)

This one occurred several times when I was being vaporized by police, after my ship's energy reached zero.

[cag]

Ok, I fixed the error but an AI guru will have to judge my solution. Is the oddsAssessment relevant after a player's ship is destroyed and the player was a member of a group/escortGroup?

New version uploaded, same link

https://www.dropbox.com/s/cudwuq8d56ep ... .zip?dl=0