Tiled: seed object GUIDs past the map's ids when it has no nextobjectid

packages/melonjs/CHANGELOG.md

@@ -25,6 +25,7 @@
 - UI: `UITextButton#bitmapText`, for `label`
 
 ### Fixed
+- Tiled: a map without `nextobjectid` (optional in the TMX format, left out by hand-made or generated maps) seeds the GUID counter past the highest object id it holds, group layers included. The counter started at 0, so objects added without an id took the GUIDs of the map's own objects, and the builtin detector, which keys contacts by GUID, dropped the second contact in a step that shared a key with another: a player fell through an enemy it shared a key with ([#1809](https://github.com/melonjs/melonJS/issues/1809))
 - Camera: `fadeIn()` / `fadeOut()` (and a `FadeEffect`, so the fade state transition and a fading level trigger too) always call `onComplete`. The fade counted as done once its colour's alpha read as the target, which a 32-bit float can do one step before the tween ends; the camera then removed the effect, stopping the tween before it called `onComplete`, so code waiting on it stayed stuck. The fade now ends when its tween does ([#1810](https://github.com/melonjs/melonJS/issues/1810))
 - Rendering: on WebGL and WebGPU, a clip inside a clip is the overlap of the two, as it already was on Canvas. A clipping container nested in another, or a scrolling `UIPanel` in a clipping parent, drew past the outer clip wherever it stuck out of it, because the inner `clipRect` replaced the outer scissor ([#1816](https://github.com/melonjs/melonJS/issues/1816)). The clipping example shows the case
 - UI: a radio group holds one choice from when its checkboxes are made, not only once they are in the scene: before, several could be checked at once until they were added, and `UICheckbox.checkedIn()` answered `undefined`. A checkbox leaves its group when destroyed; groups hold their members weakly, so one dropped without being destroyed can still be collected

packages/melonjs/src/level/tiled/TMXTileMap.js

@@ -152,6 +152,33 @@ function refreshAbsoluteBounds(container) {
 	});
 }
 
+/**
+ * the highest object id in a map's layers, group layers included
+ * @param {object[]} [layers] - the map's (or a group's) layer data
+ * @returns {number} the highest id, or 0 when there is none
+ * @ignore
+ */
+function highestObjectId(layers) {
+	let highest = 0;
+	if (!Array.isArray(layers)) {
+		return highest;
+	}
+	for (const layer of layers) {
+		if (Array.isArray(layer.objects)) {
+			for (const object of layer.objects) {
+				const id = +object.id;
+				if (id > highest) {
+					highest = id;
+				}
+			}
+		}
+		if (Array.isArray(layer.layers)) {
+			highest = Math.max(highest, highestObjectId(layer.layers));
+		}
+	}
+	return highest;
+}
+
 /**
  * a TMX Tile Map Object
  * Tiled QT +0.7.x format
@@ -294,8 +321,11 @@ export default class TMXTileMap {
 		// Check if map is from melon editor
 		this.isEditor = data.editor === "melon-editor";
 
-		// object id
-		this.nextobjectid = +data.nextobjectid || undefined;
+		// the first id free for objects added at runtime: what the map says,
+		// or, as Tiled maps may leave it out, one past the highest object id
+		// actually in it, so a GUID made from the counter cannot meet one made
+		// from a map object's id (#1809)
+		this.nextobjectid = +data.nextobjectid || highestObjectId(data.layers) + 1;
 
 		// hex/iso properties
 		this.hexsidelength = +data.hexsidelength;

packages/melonjs/tests/tmx-nextobjectid.spec.js

@@ -0,0 +1,107 @@
+/**
+ * A Tiled map without `nextobjectid` (#1809).
+ *
+ * Tiled always writes `nextobjectid` on `<map>`, but the TMX format makes it
+ * optional and hand-made or generated maps leave it out. `level.load()`
+ * seeds the GUID counter with it, so with it missing the counter started at
+ * 0, and objects added without an id took GUIDs equal to the map objects'
+ * own (`<level>-<id>`). The builtin detector keys contacts by GUID, so two
+ * different pairs shared a key and the second contact in a step was dropped:
+ * in Alex 4, Alex fell through the boss.
+ *
+ * The map now seeds the counter past the highest object id it holds.
+ */
+import { describe, expect, it } from "vitest";
+import { TMXTileMap, utils } from "../src/index.js";
+
+/** a map's JSON with the given layers, and `nextobjectid` if given */
+const mapJSON = (layers, nextobjectid) => {
+	const data = {
+		width: 10,
+		height: 10,
+		tilewidth: 32,
+		tileheight: 32,
+		orientation: "orthogonal",
+		renderorder: "right-down",
+		infinite: false,
+		version: "1.10",
+		tiledversion: "1.12.0",
+		tilesets: [],
+		layers,
+	};
+	if (nextobjectid !== undefined) {
+		data.nextobjectid = nextobjectid;
+	}
+	return data;
+};
+
+/** an object layer holding objects with the given ids */
+const objects = (...ids) => {
+	return {
+		type: "objectgroup",
+		name: "objects",
+		visible: true,
+		opacity: 1,
+		objects: ids.map((id) => {
+			return { id, name: "", x: 0, y: 0, width: 16, height: 16 };
+		}),
+	};
+};
+
+describe("TMXTileMap#nextobjectid (#1809)", () => {
+	it("is the map's own value when it has one", () => {
+		const map = new TMXTileMap("with", mapJSON([objects(1, 2)], 20));
+		expect(map.nextobjectid).toBe(20);
+	});
+
+	it("without one, is one past the highest object id in the map", () => {
+		const map = new TMXTileMap("without", mapJSON([objects(3, 47, 7)]));
+		expect(map.nextobjectid).toBe(48);
+	});
+
+	it("counts objects inside group layers, at any depth", () => {
+		const layers = [
+			objects(5),
+			{
+				type: "group",
+				name: "boss room",
+				layers: [
+					objects(9),
+					{ type: "group", name: "inner", layers: [objects(31)] },
+				],
+			},
+		];
+		const map = new TMXTileMap("groups", mapJSON(layers));
+		expect(map.nextobjectid).toBe(32);
+	});
+
+	it("is 1 for a map with no objects", () => {
+		const tiles = {
+			type: "tilelayer",
+			name: "ground",
+			width: 1,
+			height: 1,
+			data: [0],
+		};
+		expect(new TMXTileMap("empty", mapJSON([tiles])).nextobjectid).toBe(1);
+	});
+
+	it("so GUIDs of objects added at runtime never meet those of the map's objects", () => {
+		// the Alex 4 case: the boss and the player have no id of their own,
+		// the cherries and stars in the map do
+		const ids = [1, 2, 3, 45, 46, 47];
+		const map = new TMXTileMap("level12", mapJSON([objects(...ids)]));
+		// as level.load() does
+		utils.resetGUID("level12", map.nextobjectid);
+		const authored = ids.map((id) => {
+			return utils.createGUID(id);
+		});
+		const spawned = Array.from({ length: 60 }, () => {
+			return utils.createGUID();
+		});
+		const shared = spawned.filter((guid) => {
+			return authored.includes(guid);
+		});
+		expect(shared).toEqual([]);
+	});
+});