Merge pull request #590 from Hornwitser/bugfixes-3

Bugfixes 3

.eslintrc.js

@@ -320,7 +320,7 @@ module.exports = {
 		"semi-style": "off",
 		"sort-imports": "off",
 		"sort-keys": "off",
-		"sort-vars": "error",
+		"sort-vars": "off",
 		"space-before-blocks": "error",
 		"space-before-function-paren": "off",
 		"space-in-parens": ["error", "never"],

CHANGELOG.md

@@ -11,6 +11,10 @@
 - Fixed Web UI not showing instance saves when the permission to list and subscribe the saves is granted but the user is not a cluster admin.
 - Similarly fixed Web UI not showing the mod pack create, delete, list and host revoke token actions if the user is not a cluster admin.
 - Fixed Research Sync applying progress of a previous techology level to the level on the controller when it's the active research during instance startup.
+- Fixed data export with a mod using shorthand notation for the default value of a color-setting causing the ModPack data to be set to invalid data.
+- Fixed per user per instance player stats not being deleted when the instance it's for is deleted.
+- Fixed mods setting the item prototype icons property to an object mapping instead of an array breaking the data export.
+- Fixed a bad mod in the host's mods folder causing instance start to fail instead of the host re-downloading the mod.
 
 
 ## Version 2.0.0-alpha.16

packages/controller/src/Controller.ts

@@ -964,6 +964,7 @@ export default class Controller {
 		this.instancesDirty = true;
 
 		this.clearSavesOfInstance(instanceId);
+		this.userManager.clearStatsOfInstance(instanceId);
 
 		let prev = instance.status;
 		instance.status = "deleted";

packages/controller/src/UserManager.ts

@@ -113,4 +113,13 @@ export default class UserManager {
 			this.onlineUsers.delete(user);
 		}
 	}
+
+	clearStatsOfInstance(instanceId: number) {
+		for (const user of this.users.values()) {
+			this.notifyLeave(user, instanceId);
+			user.instanceStats.delete(instanceId);
+			user.recalculatePlayerStats();
+		}
+		this.dirty = true;
+	}
 }

packages/host/src/Host.ts

@@ -600,8 +600,9 @@ export default class Host extends lib.Link {
 		try {
 			modInfo = await this.modStore.loadMod(mod.name, mod.version);
 		} catch (err: any) {
+			// A broken zip file may have been left in the mods folder
 			if (err.code !== "ENOENT") {
-				throw err;
+				logger.error(`Error loading mod ${mod.name} ${mod.version}: ${err.message}`);
 			}
 		}
 		if (modInfo && (!mod.sha1 || mod.sha1 === modInfo.sha1)) {

packages/host/src/export.ts

@@ -18,11 +18,10 @@ interface SimpleIconSpecification {
 	icon_size: number;
 	icon_mipmaps?: number;
 }
-type Color = { r: number, g: number, b: number, a: number };
 interface IconLayer {
 	icon: string;
 	icon_size: number;
-	tint?: Color,
+	tint?: lib.FactorioColor,
 	shift?: [number, number],
 	scale?: number,
 	icon_mipmaps?: number,
@@ -202,15 +201,9 @@ async function loadLayeredIcon(
 
 		iconLayer = iconLayer.clone();
 
-		let tint: Color;
+		let tint: lib.Color;
 		if (layer.tint) {
-			let divisor = (layer.tint.r > 1 || layer.tint.g > 1 || layer.tint.b > 1) ? 255 : 1;
-			tint = {
-				r: (layer.tint.r || 0) / divisor,
-				g: (layer.tint.g || 0) / divisor,
-				b: (layer.tint.b || 0) / divisor,
-				a: layer.tint.a || 1,
-			};
+			tint = lib.normalizeColor(layer.tint);
 		} else {
 			tint = { r: 1, b: 1, g: 1, a: 1 };
 		}
@@ -252,6 +245,18 @@ async function loadLayeredIcon(
 	return icon;
 }
 
+function fixIcons(item: ItemPrototype) {
+	const icons = item.icons;
+	if (typeof icons === "object" && !(icons instanceof Array) && icons !== null) {
+		// It's possible to specify icons as an object with arbitrary keys
+		// and the game will still accept it, cast the value to array if
+		// this is the case.
+		item = { ...item };
+		item.icons = Object.values(icons);
+	}
+	return item;
+}
+
 const itemTypes = new Set([
 	"item",
 	"ammo",
@@ -284,7 +289,8 @@ const itemTypes = new Set([
 function filterItems(prototypes: Prototypes): ItemPrototype[] {
 	return Object.entries(prototypes)
 		.filter(([type, _]) => itemTypes.has(type))
-		.flatMap(([_, typePrototypes]) => Object.values(typePrototypes)) as ItemPrototype[]
+		.flatMap(([_, typePrototypes]) => Object.values(typePrototypes) as ItemPrototype[])
+		.map(fixIcons)
 	;
 }
 

packages/lib/browser.ts

@@ -6,6 +6,7 @@ export * from "./src/factorio/exchange_string";
 export * from "./src/helpers";
 export * from "./src/ini";
 export * from "./src/link";
+export * from "./src/lua_tools";
 export * from "./src/logging";
 export * from "./src/permissions";
 export * from "./src/plugin";

packages/lib/src/data/ModPack.ts

@@ -2,6 +2,7 @@ import zlib from "zlib";
 import { Type, Static } from "@sinclair/typebox";
 
 import * as libSchema from "../schema";
+import * as libLuaTools from "../lua_tools";
 
 import ExportManifest from "./ExportManifest";
 import { integerFactorioVersion } from "./version";
@@ -363,7 +364,11 @@ export default class ModPack {
 				continue;
 			}
 
-			this.settings[settingType].set(prototype.name, { value: prototype.default_value });
+			let value = prototype.default_value;
+			if (prototype.type === "color-setting") {
+				value = libLuaTools.normalizeColor(value);
+			}
+			this.settings[settingType].set(prototype.name, { value });
 		}
 	}
 }

packages/lib/src/lua_tools.ts

@@ -3,6 +3,54 @@
  * @module lib/lua_tools
  */
 
+/**
+ * A colour as it may originate from Factorio Lua code, see
+ * https://lua-api.factorio.com/latest/types/Color.html
+ */
+export type FactorioColor = { r?: number, g?: number, b?: number, a?: number } | [number, number, number, number?];
+
+export type Color = { r: number, g: number, b: number, a: number };
+
+/**
+ * Normalise a color as it may appear in Factorio Lua code to be an object
+ * with r, g, b, a properties in the range 0 to 1.
+ *
+ * @param color -
+ *     A colour that may be specified with left out components or as an
+ *     array and have the range 1 to 0 or 255 to 0.
+ * @returns
+ *     Normalised colour where all 4 components are present as properties in
+ *     the range 0 to 1.
+ */
+export function normalizeColor(color: FactorioColor) {
+	let r, g, b, a;
+	if (color instanceof Array) {
+		([r, g, b, a] = color);
+	} else {
+		({ r, g, b, a } = color);
+	}
+
+	r ??= NaN;
+	g ??= NaN;
+	b ??= NaN;
+	a ??= NaN;
+
+	// Note NaN > 1 is false and NaN / 255 is NaN
+	if (r > 1 || g > 1 || b > 1 || a > 1) {
+		r /= 255;
+		g /= 255;
+		b /= 255;
+		a /= 255;
+	}
+
+	return {
+		r: Number.isNaN(r) ? 0 : r,
+		g: Number.isNaN(g) ? 0 : g,
+		b: Number.isNaN(b) ? 0 : b,
+		a: Number.isNaN(a) ? 1 : a,
+	};
+}
+
 /**
  * Escapes a string for inclusion into a lua string
  *

packages/lib/tsconfig.browser.json

@@ -12,6 +12,7 @@
 		"./src/hash.ts",
 		"./src/ini.ts",
 		"./src/link/*.ts",
+		"./src/lua_tools.ts",
 		"./src/logging.ts",
 		"./src/permissions.ts",
 		"./src/prometheus.ts",

packages/web_ui/src/components/ModPackViewPage.tsx

@@ -202,7 +202,7 @@ function SearchModsTable(props: SearchModsTableProps) {
 						align: "right",
 						render: (_, result) => <Select
 							size="small"
-							bordered={false}
+							variant="borderless"
 							value={modResultSelectedVersion.get(result.name) || 0}
 							onChange={index => {
 								const newVersions = new Map(modResultSelectedVersion);

test/controller/UserManager.js

@@ -1,5 +1,6 @@
 "use strict";
 const assert = require("assert").strict;
+const { PlayerStats } = require("@clusterio/lib");
 const { ControllerUser, UserManager } = require("@clusterio/controller");
 
 describe("controller/src/UserManager", function() {
@@ -31,5 +32,28 @@ describe("controller/src/UserManager", function() {
 			assert(!userManager.onlineUsers.has(user));
 			assert.deepEqual(user.instances, new Set());
 		});
+		describe(".clearStatsOfInstance()", function() {
+			it("should remove the instance stats from existing users", function() {
+				let user1 = ControllerUser.fromJSON({ name: "admin", roles: [1] });
+				let user2 = ControllerUser.fromJSON({ name: "player", roles: [1] });
+				userManager.users.set(user1.name, user1);
+				userManager.users.set(user2.name, user2);
+				user1.instanceStats.set(10, new PlayerStats({ join_count: 1 }));
+				user1.instanceStats.set(11, new PlayerStats({ join_count: 6 }));
+				user1.recalculatePlayerStats();
+				userManager.notifyJoin(user1, 11);
+				user2.instanceStats.set(10, new PlayerStats({ join_count: 3 }));
+				user2.recalculatePlayerStats();
+				assert.equal(user1.playerStats.joinCount, 7);
+				assert.equal(user2.playerStats.joinCount, 3);
+				assert(userManager.onlineUsers.has(user1));
+
+				userManager.clearStatsOfInstance(11);
+
+				assert.equal(user1.playerStats.joinCount, 1);
+				assert(!userManager.onlineUsers.has(user1));
+				assert.equal(user2.playerStats.joinCount, 3);
+			});
+		});
 	});
 });

test/lib/lua_tools.js

@@ -5,6 +5,25 @@ const lib = require("@clusterio/lib");
 
 
 describe("lib/lua_tools", function() {
+	describe("normalizeColor()", function() {
+		it("should normalise colours according to the documentation", function() {
+			const nc = lib.normalizeColor;
+			assert.deepEqual(nc({ r: 1, g: 0.2, b: 0.4, a: 0.8 }), { r: 1, g: 0.2, b: 0.4, a: 0.8 });
+			assert.deepEqual(nc({ r: 1, g: 0.2, b: 0.4 }), { r: 1, g: 0.2, b: 0.4, a: 1 });
+			assert.deepEqual(nc({ r: 1, b: 0.4 }), { r: 1, g: 0, b: 0.4, a: 1 });
+			assert.deepEqual(nc({ a: 0.4 }), { r: 0, g: 0, b: 0, a: 0.4 });
+			assert.deepEqual(nc({}), { r: 0, g: 0, b: 0, a: 1 });
+			assert.deepEqual(nc([1, 0.2, 0.4, 0.8]), { r: 1, g: 0.2, b: 0.4, a: 0.8 });
+			assert.deepEqual(nc([1, 0.2, 0.4]), { r: 1, g: 0.2, b: 0.4, a: 1 });
+			assert.deepEqual(nc({ r: 255, g: 51, b: 102, a: 204 }), { r: 1, g: 0.2, b: 0.4, a: 0.8 });
+			assert.deepEqual(nc({ r: 255, g: 51, b: 102 }), { r: 1, g: 0.2, b: 0.4, a: 1 });
+			assert.deepEqual(nc({ r: 255, b: 102 }), { r: 1, g: 0, b: 0.4, a: 1 });
+			assert.deepEqual(nc({ a: 102 }), { r: 0, g: 0, b: 0, a: 0.4 });
+			assert.deepEqual(nc({}), { r: 0, g: 0, b: 0, a: 1 });
+			assert.deepEqual(nc([255, 51, 102, 204]), { r: 1, g: 0.2, b: 0.4, a: 0.8 });
+			assert.deepEqual(nc([255, 51, 102]), { r: 1, g: 0.2, b: 0.4, a: 1 });
+		});
+	});
 	describe("escapeString()", function() {
 		it("should escape backslashes", function() {
 			assert.equal(lib.escapeString("\\a"), "\\\\a");