From eb39e4393d715c9075e74c861b34cab66130ae19 Mon Sep 17 00:00:00 2001 From: daddy Date: Mon, 14 Sep 2026 19:56:44 +0000 Subject: [PATCH] Fix 3 real client/server chunk-codec interop bugs, caught by a new golden-fixture test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No test ever exercised the client ChunkDataDecoder against real byte data before this (M2 chunk streaming was never fed real content on PC2/PC3). Added scripts/smoke_test_chunk_codec_interop.gd, decoding the backend's exact golden fixtures (backend/tests/fixtures/chunks/*.bin) and asserting byte-identical logical values -- the real cross-language contract this whole persistence system depends on. It found three bugs on first run: 1. chunk_data_decoder.gd's _read_float32/_write_float32 used `bits as float`/`val as int`, a numeric cast in GDScript, not an IEEE-754 bitcast -- silently corrupted every non-integer position/rotation/ scale/bounds value (1.5 decoded as 1069547520.0, verified). Fixed with PackedByteArray.decode_float/encode_float (real bitcast, same little-endian order as the rest of this format). 2. backend/app/services/chunk_codec.py's binary codec wrote bounds as min+size, disagreeing with world.md §3's documented min+max wire format and with the client's actual (correct) decoder -- corrupted every chunk's bounds on the wire despite the backend's own JSON path using min+max correctly. Fixed the binary encode/decode to match the spec; regenerated the golden fixtures. 3. chunk_data_decoder.gd's object-properties parsing used `JSON.parse_string(props_str) or {}` -- GDScript's `or` is a boolean operator, not Python/JS-style short-circuit-return, so this silently evaluated to a bool for any object with real property data, crashing on assignment to the typed `properties: Dictionary` field. Fixed with an explicit type check. Full backend suite (132/132) and 4 sibling client smoke tests re-verified clean after all three fixes; no other `or {}`/`or []` instances of bug 3's pattern found elsewhere in the codebase. --- backend/app/services/chunk_codec.py | 22 ++-- .../fixtures/chunks/chunk_with_objects.bin | Bin 459 -> 459 bytes .../fixtures/chunks/simple_tile_chunk.bin | Bin 264 -> 264 bytes scripts/chunk_data_decoder.gd | 23 +++- scripts/smoke_test_chunk_codec_interop.gd | 107 ++++++++++++++++++ 5 files changed, 139 insertions(+), 13 deletions(-) create mode 100644 scripts/smoke_test_chunk_codec_interop.gd diff --git a/backend/app/services/chunk_codec.py b/backend/app/services/chunk_codec.py index 41437b2..9ec8845 100644 --- a/backend/app/services/chunk_codec.py +++ b/backend/app/services/chunk_codec.py @@ -127,10 +127,15 @@ class WorldObject: @dataclass class ChunkBounds: - """Wire representation is min-corner + size (matching the Godot AABB - the reference encoder builds from), not min/max as world.md §3.1's - logical-model prose describes -- both are fully recoverable from each - other; this is just the on-the-wire convention.""" + """Stored here as min-corner + size (matching the Godot client's AABB + type) for convenience, but the §3.6 BINARY wire format is min + max + (world.md §3.1, and the client's actual ChunkDataDecoder) -- fixed + 2026-09-14 after a real client/server interop test caught this class's + original binary codec writing min+size on the wire, which the client + decoder (unaware of the "size" convention) read as a second min/max + pair, corrupting every chunk's bounds. The two representations are + fully recoverable from each other; `max` below is computed for exactly + this reason.""" min: tuple[float, float, float] = (0.0, 0.0, 0.0) size: tuple[float, float, float] = (0.0, 0.0, 0.0) @@ -237,9 +242,9 @@ def _encode_metadata(payload: ChunkPayload) -> bytes: out += _w_f32(payload.bounds.min[0]) out += _w_f32(payload.bounds.min[1]) out += _w_f32(payload.bounds.min[2]) - out += _w_f32(payload.bounds.size[0]) - out += _w_f32(payload.bounds.size[1]) - out += _w_f32(payload.bounds.size[2]) + out += _w_f32(payload.bounds.max[0]) + out += _w_f32(payload.bounds.max[1]) + out += _w_f32(payload.bounds.max[2]) return bytes(out) @@ -250,7 +255,8 @@ def _decode_metadata(data: bytes, result: ChunkPayload) -> None: coord = (_read_i32(data, off), _read_i32(data, off + 4), _read_i32(data, off + 8)) off += 12 min_pos = (_read_f32(data, off), _read_f32(data, off + 4), _read_f32(data, off + 8)) - size = (_read_f32(data, off + 12), _read_f32(data, off + 16), _read_f32(data, off + 20)) + max_pos = (_read_f32(data, off + 12), _read_f32(data, off + 16), _read_f32(data, off + 20)) + size = tuple(a - b for a, b in zip(max_pos, min_pos)) result.coord = coord result.bounds = ChunkBounds(min=min_pos, size=size) diff --git a/backend/tests/fixtures/chunks/chunk_with_objects.bin b/backend/tests/fixtures/chunks/chunk_with_objects.bin index b9a00ec81a591f55a3e4545775b3b4ed0463d5e1..f87c8aa9393724347e509dc69f2313c9039df695 100644 GIT binary patch delta 66 zcmX@je45$7(LKPKkwJq21ZFcbF#Mm#efr`4`b!Um5+>cBlw>RabHizdqtjRIpZ9V{ W{@t3ii3VD%&I}9(4sDFjVFUmz{~G-O delta 66 zcmX@je45$7(LKPKkwJq21ZFcbFdUX-VOi_fCcbx{R>W(bPsT5WYAy$fg3>V|80WH*{7BlI?jq~oP6yPuR>W(bPsT5WYAy$fg3;^lsJEFa?jz9GljnYnOS&q6X(+nC SOo538TC7e$bxsrGR{{X==oyay diff --git a/scripts/chunk_data_decoder.gd b/scripts/chunk_data_decoder.gd index 9e19863..2a08d3a 100644 --- a/scripts/chunk_data_decoder.gd +++ b/scripts/chunk_data_decoder.gd @@ -404,7 +404,15 @@ static func _parse_object_section(data: PackedByteArray, result: ChunkPayload) - offset += 2 var props_str := data.slice(offset, offset + props_len).get_string_from_utf8() offset += props_len - obj.properties = JSON.parse_string(props_str) or {} + # `or` in GDScript is a boolean operator, not Python/JS-style + # short-circuit-return -- `parsed or {}` returns a bool (true for any + # non-empty Dictionary), not `parsed` itself. Assigning that bool to + # `properties` (typed Dictionary) crashes at runtime for any object + # with real property data -- verified: a real fixture with + # {"locked": false, ...} threw "Invalid assignment of property or key + # 'properties' with value of type 'bool'". + var parsed_props: Variant = JSON.parse_string(props_str) + obj.properties = parsed_props if parsed_props is Dictionary else {} # flags (ContentFlags) obj.flags.violence = data[offset] @@ -607,8 +615,11 @@ static func _read_int64(data: PackedByteArray, offset: int) -> int: return hi * 0x100000000 + lo static func _read_float32(data: PackedByteArray, offset: int) -> float: - var bits := _read_uint32(data, offset) - return bits as float # GDScript float is 64-bit; bits repr works for IEEE 754 + # `int as float` is a NUMERIC cast, not an IEEE-754 bit reinterpretation -- + # it was silently corrupting every non-integer value (verified: bits for + # 1.5f decoded as 1069547520.0). decode_float() does a real bitcast and + # matches this format's little-endian byte order (§3.6). + return data.decode_float(offset) static func _read_hex_string(data: PackedByteArray, offset: int, length: int) -> String: var hex_chars := "0123456789abcdef" @@ -637,8 +648,10 @@ static func _write_int64(val: int) -> PackedByteArray: return _write_uint32(lo) + _write_uint32(hi) static func _write_float32(val: float) -> PackedByteArray: - var bits := val as int - return _write_uint32(bits) + var out := PackedByteArray() + out.resize(4) + out.encode_float(0, val) + return out static func _write_string(s: String) -> PackedByteArray: var bytes := s.to_utf8_buffer() diff --git a/scripts/smoke_test_chunk_codec_interop.gd b/scripts/smoke_test_chunk_codec_interop.gd new file mode 100644 index 0000000..9548125 --- /dev/null +++ b/scripts/smoke_test_chunk_codec_interop.gd @@ -0,0 +1,107 @@ +extends SceneTree +## Proves the client-side ChunkDataDecoder decodes the backend's exact golden +## fixture files (backend/tests/fixtures/chunks/*.bin) with byte-identical +## logical values -- the actual cross-language contract this whole +## persistence system depends on. Also regression-covers the float32 +## int<->float numeric-cast bug (fixed alongside this test): `bits as float` +## silently corrupted every non-integer value (1.5 decoded as 1069547520.0). +## +## Run: ~/godot/godot --headless --script res://scripts/smoke_test_chunk_codec_interop.gd + +const FIXTURES_DIR := "res://backend/tests/fixtures/chunks/" + +var passed := 0 +var failed := 0 + + +func _check(label: String, cond: bool) -> void: + if cond: + passed += 1 + print(" PASS: ", label) + else: + failed += 1 + print(" FAIL: ", label) + + +func _init() -> void: + _test_float32_roundtrip() + _test_empty_chunk_fixture() + _test_simple_tile_chunk_fixture() + _test_chunk_with_objects_fixture() + + print("\n=== SMOKE RESULT: %d/%d passed ===" % [passed, passed + failed]) + quit(0 if failed == 0 else 1) + + +func _read_fixture(name: String) -> PackedByteArray: + var f := FileAccess.open(FIXTURES_DIR + name, FileAccess.READ) + if f == null: + push_error("Could not open fixture: " + name) + return PackedByteArray() + return f.get_buffer(f.get_length()) + + +func _test_float32_roundtrip() -> void: + print("--- float32 bitcast regression (the actual bug this fix addresses) ---") + for val in [1.5, -110.25, 0.5, 3.14159, -0.001, 0.0, 100.0]: + var encoded: PackedByteArray = ChunkDataDecoder._write_float32(val) + var decoded: float = ChunkDataDecoder._read_float32(encoded, 0) + _check("float32 round-trip for %s (got %s)" % [val, decoded], is_equal_approx(decoded, val)) + # The specific regression case: 1.5 must NOT decode as 1069547520.0 + # (the old `bits as float` numeric-cast bug's actual observed output). + var bad_encoded := ChunkDataDecoder._write_float32(1.5) + var bad_decoded: float = ChunkDataDecoder._read_float32(bad_encoded, 0) + _check("1.5 does not decode as the old numeric-cast bug's 1069547520.0", not is_equal_approx(bad_decoded, 1069547520.0)) + + +func _test_empty_chunk_fixture() -> void: + print("--- backend golden fixture: empty_chunk.bin ---") + var data := _read_fixture("empty_chunk.bin") + _check("fixture file read (non-empty)", data.size() > 0) + var payload: ChunkDataDecoder.ChunkPayload = ChunkDataDecoder.decode_binary(data) + _check("decoded without error", payload != null) + if payload == null: + return + _check("world_id matches backend fixture (7)", payload.world_id == "7") + _check("no tile layers", payload.tile_layers.is_empty()) + _check("no objects", payload.objects.is_empty()) + _check("revision matches backend fixture (1)", payload.revision == 1) + + +func _test_simple_tile_chunk_fixture() -> void: + print("--- backend golden fixture: simple_tile_chunk.bin ---") + var data := _read_fixture("simple_tile_chunk.bin") + var payload: ChunkDataDecoder.ChunkPayload = ChunkDataDecoder.decode_binary(data) + _check("decoded without error", payload != null) + if payload == null: + return + _check("world_id matches (12)", payload.world_id == "12") + _check("coord matches (3,-1,0)", payload.coord == Vector3i(3, -1, 0)) + _check("bounds.position matches (96.0,-32.0,0.0) -- non-trivial floats", payload.bounds.position.is_equal_approx(Vector3(96.0, -32.0, 0.0))) + _check("bounds.size matches (32.0,32.0,1.0)", payload.bounds.size.is_equal_approx(Vector3(32.0, 32.0, 1.0))) + _check("one tile layer parsed", payload.tile_layers.size() == 1) + if payload.tile_layers.size() == 1: + var layer: ChunkDataDecoder.TileLayerData = payload.tile_layers[0] + _check("tile layer dims 4x4x1", layer.width == 4 and layer.height == 4 and layer.depth == 1) + _check("tile data matches backend fixture", layer.tiles == PackedInt32Array([0, 1, 1, 0, 1, 2, 2, 1, 1, 2, 2, 1, 0, 1, 1, 0])) + _check("revision matches (2)", payload.revision == 2) + + +func _test_chunk_with_objects_fixture() -> void: + print("--- backend golden fixture: chunk_with_objects.bin (real non-integer floats) ---") + var data := _read_fixture("chunk_with_objects.bin") + var payload: ChunkDataDecoder.ChunkPayload = ChunkDataDecoder.decode_binary(data) + _check("decoded without error", payload != null) + if payload == null: + return + _check("one object parsed", payload.objects.size() >= 1) + if payload.objects.is_empty(): + return + var obj: ChunkDataDecoder.WorldObjectData = payload.objects[0] + _check("object id matches (obj_0001)", obj.id == "obj_0001") + _check("object asset_id matches (props/chest)", obj.asset_id == "props/chest") + # The core regression check: these are exactly the kind of non-integer + # values that decoded as garbage under the old `bits as float` bug. + _check("object position.x matches 101.5 exactly (was the corrupted value)", is_equal_approx(obj.position.x, 101.5)) + _check("object position.y matches -110.25 exactly (was the corrupted value)", is_equal_approx(obj.position.y, -110.25)) + _check("object position.z matches 0.5 exactly (was the corrupted value)", is_equal_approx(obj.position.z, 0.5))