From f769c6c5f5b42614266b19dfe5cd808da2ec9b15 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Fri, 28 Nov 2025 21:26:32 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20Remove=20O(n=C2=B2)=20list-building=20fr?= =?UTF-8?q?om=20compute=5Ffov()=20(closes=20#146)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compute_fov() was iterating through the entire grid to build a Python list of visible cells, causing O(grid_size) performance instead of O(radius²). On a 1000×1000 grid this was 15.76ms vs 0.48ms. The fix returns None instead - users should use is_in_fov() to query visibility, which is the pattern already used by existing code. Performance: 33x speedup (15.76ms → 0.48ms on 1M cell grid) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/UIGrid.cpp | 39 +----- tests/benchmarks/tcod_fov_isolated.py | 99 +++++++++++++++ .../regression/issue_146_fov_returns_none.py | 114 ++++++++++++++++++ 3 files changed, 217 insertions(+), 35 deletions(-) create mode 100644 tests/benchmarks/tcod_fov_isolated.py create mode 100644 tests/regression/issue_146_fov_returns_none.py diff --git a/src/UIGrid.cpp b/src/UIGrid.cpp index 1bfbb6a..abdd92e 100644 --- a/src/UIGrid.cpp +++ b/src/UIGrid.cpp @@ -1183,41 +1183,10 @@ PyObject* UIGrid::py_compute_fov(PyUIGridObject* self, PyObject* args, PyObject* // Compute FOV self->data->computeFOV(x, y, radius, light_walls, (TCOD_fov_algorithm_t)algorithm); - - // Build list of visible cells as tuples (x, y, visible, discovered) - PyObject* result_list = PyList_New(0); - if (!result_list) return NULL; - - // Iterate through grid and collect visible cells - for (int gy = 0; gy < self->data->grid_y; gy++) { - for (int gx = 0; gx < self->data->grid_x; gx++) { - if (self->data->isInFOV(gx, gy)) { - // Create tuple (x, y, visible, discovered) - PyObject* cell_tuple = PyTuple_New(4); - if (!cell_tuple) { - Py_DECREF(result_list); - return NULL; - } - - PyTuple_SET_ITEM(cell_tuple, 0, PyLong_FromLong(gx)); - PyTuple_SET_ITEM(cell_tuple, 1, PyLong_FromLong(gy)); - PyTuple_SET_ITEM(cell_tuple, 2, Py_True); // visible - PyTuple_SET_ITEM(cell_tuple, 3, Py_True); // discovered - Py_INCREF(Py_True); // Need to increment ref count for True - Py_INCREF(Py_True); - - // Append to list - if (PyList_Append(result_list, cell_tuple) < 0) { - Py_DECREF(cell_tuple); - Py_DECREF(result_list); - return NULL; - } - Py_DECREF(cell_tuple); // List now owns the reference - } - } - } - - return result_list; + + // Return None - use is_in_fov() to query visibility + // See issue #146: returning a list had O(grid_size) performance + Py_RETURN_NONE; } PyObject* UIGrid::py_is_in_fov(PyUIGridObject* self, PyObject* args) diff --git a/tests/benchmarks/tcod_fov_isolated.py b/tests/benchmarks/tcod_fov_isolated.py new file mode 100644 index 0000000..b15af17 --- /dev/null +++ b/tests/benchmarks/tcod_fov_isolated.py @@ -0,0 +1,99 @@ +#!/usr/bin/env python3 +""" +Isolated FOV benchmark - test if the slowdown is TCOD or Python wrapper +""" +import mcrfpy +import sys +import time + +def run_test(runtime): + print("=" * 60) + print("FOV Isolation Test - Is TCOD slow, or is it the Python wrapper?") + print("=" * 60) + + # Create a 1000x1000 grid + mcrfpy.createScene("test") + ui = mcrfpy.sceneUI("test") + texture = mcrfpy.Texture("assets/kenney_ice.png", 16, 16) + + print("\nCreating 1000x1000 grid...") + t0 = time.perf_counter() + grid = mcrfpy.Grid(pos=(0,0), size=(800,600), grid_size=(1000, 1000), texture=texture) + ui.append(grid) + print(f" Grid creation: {(time.perf_counter() - t0)*1000:.1f}ms") + + # Set walkability + print("Setting walkability (this takes a while)...") + t0 = time.perf_counter() + for y in range(0, 1000, 10): # Sample every 10th row for speed + for x in range(1000): + cell = grid.at(x, y) + cell.walkable = True + cell.transparent = True + print(f" Partial walkability: {(time.perf_counter() - t0)*1000:.1f}ms") + + # Test 1: compute_fov (now returns None - fast path after #146 fix) + print("\n--- Test 1: grid.compute_fov() [returns None after #146 fix] ---") + times = [] + for i in range(5): + t0 = time.perf_counter() + result = grid.compute_fov(500, 500, radius=15) + elapsed = (time.perf_counter() - t0) * 1000 + times.append(elapsed) + # Count visible cells using is_in_fov (the correct pattern) + visible = sum(1 for dy in range(-15, 16) for dx in range(-15, 16) + if 0 <= 500+dx < 1000 and 0 <= 500+dy < 1000 + and grid.is_in_fov(500+dx, 500+dy)) + print(f" Run {i+1}: {elapsed:.3f}ms, result={result}, ~{visible} visible cells") + print(f" Average: {sum(times)/len(times):.3f}ms") + + # Test 2: Just check is_in_fov for cells in radius (what rendering would do) + print("\n--- Test 2: Simulated render check (only radius cells) ---") + times = [] + for i in range(5): + # First compute FOV (we need to do this) + grid.compute_fov(500, 500, radius=15) + + # Now simulate what rendering would do - check only nearby cells + t0 = time.perf_counter() + visible_count = 0 + for dy in range(-15, 16): + for dx in range(-15, 16): + x, y = 500 + dx, 500 + dy + if 0 <= x < 1000 and 0 <= y < 1000: + if grid.is_in_fov(x, y): + visible_count += 1 + elapsed = (time.perf_counter() - t0) * 1000 + times.append(elapsed) + print(f" Run {i+1}: {elapsed:.2f}ms checking ~961 cells, {visible_count} visible") + print(f" Average: {sum(times)/len(times):.2f}ms") + + # Test 3: Time just the iteration overhead (no FOV, just grid access) + print("\n--- Test 3: Grid iteration baseline (no FOV) ---") + times = [] + for i in range(5): + t0 = time.perf_counter() + count = 0 + for dy in range(-15, 16): + for dx in range(-15, 16): + x, y = 500 + dx, 500 + dy + if 0 <= x < 1000 and 0 <= y < 1000: + cell = grid.at(x, y) + if cell.walkable: + count += 1 + elapsed = (time.perf_counter() - t0) * 1000 + times.append(elapsed) + print(f" Average: {sum(times)/len(times):.2f}ms for ~961 grid.at() calls") + + print("\n" + "=" * 60) + print("CONCLUSION:") + print("After #146 fix, compute_fov() returns None instead of building") + print("a list. Test 1 and Test 2 should now have similar performance.") + print("The TCOD FOV algorithm is O(radius²) and fast.") + print("=" * 60) + + sys.exit(0) + +mcrfpy.createScene("init") +mcrfpy.setScene("init") +mcrfpy.setTimer("test", run_test, 100) diff --git a/tests/regression/issue_146_fov_returns_none.py b/tests/regression/issue_146_fov_returns_none.py new file mode 100644 index 0000000..8cecd3b --- /dev/null +++ b/tests/regression/issue_146_fov_returns_none.py @@ -0,0 +1,114 @@ +#!/usr/bin/env python3 +""" +Regression test for issue #146: compute_fov() returns None + +The compute_fov() method had O(n²) performance because it built a Python list +of all visible cells by iterating the entire grid. The fix removes this +list-building and returns None instead. Users should use is_in_fov() to query +visibility. + +Bug: 15.76ms for compute_fov() on 1000x1000 grid (iterating 1M cells) +Fix: Return None, actual FOV check via is_in_fov() takes 0.21ms +""" +import mcrfpy +import sys +import time + +def run_test(runtime): + print("=" * 60) + print("Issue #146 Regression Test: compute_fov() returns None") + print("=" * 60) + + # Create a test grid + mcrfpy.createScene("test") + ui = mcrfpy.sceneUI("test") + texture = mcrfpy.Texture("assets/kenney_ice.png", 16, 16) + + grid = mcrfpy.Grid(pos=(0,0), size=(400,300), grid_size=(50, 50), texture=texture) + ui.append(grid) + + # Set walkability for center area + for y in range(50): + for x in range(50): + cell = grid.at(x, y) + cell.walkable = True + cell.transparent = True + + # Add some walls to test blocking + for i in range(10, 20): + grid.at(i, 25).transparent = False + grid.at(i, 25).walkable = False + + print("\n--- Test 1: compute_fov() returns None ---") + result = grid.compute_fov(25, 25, radius=10) + if result is None: + print(" PASS: compute_fov() returned None") + else: + print(f" FAIL: compute_fov() returned {type(result).__name__} instead of None") + sys.exit(1) + + print("\n--- Test 2: is_in_fov() works after compute_fov() ---") + # Center should be visible + if grid.is_in_fov(25, 25): + print(" PASS: Center (25,25) is in FOV") + else: + print(" FAIL: Center should be in FOV") + sys.exit(1) + + # Cell within radius should be visible + if grid.is_in_fov(20, 25): + print(" PASS: Cell (20,25) within radius is in FOV") + else: + print(" FAIL: Cell (20,25) should be in FOV") + sys.exit(1) + + # Cell behind wall should NOT be visible + if not grid.is_in_fov(15, 30): + print(" PASS: Cell (15,30) behind wall is NOT in FOV") + else: + print(" FAIL: Cell behind wall should not be in FOV") + sys.exit(1) + + # Cell outside radius should NOT be visible + if not grid.is_in_fov(0, 0): + print(" PASS: Cell (0,0) outside radius is NOT in FOV") + else: + print(" FAIL: Cell outside radius should not be in FOV") + sys.exit(1) + + print("\n--- Test 3: Performance sanity check ---") + # Create larger grid for timing + grid_large = mcrfpy.Grid(pos=(0,0), size=(400,300), grid_size=(200, 200), texture=texture) + for y in range(0, 200, 5): # Sample for speed + for x in range(200): + cell = grid_large.at(x, y) + cell.walkable = True + cell.transparent = True + + # Time compute_fov (should be fast now - no list building) + times = [] + for i in range(5): + t0 = time.perf_counter() + grid_large.compute_fov(100, 100, radius=15) + elapsed = (time.perf_counter() - t0) * 1000 + times.append(elapsed) + + avg_time = sum(times) / len(times) + print(f" compute_fov() on 200x200 grid: {avg_time:.3f}ms avg") + + # Should be under 1ms without list building (was ~4ms with list on 200x200) + if avg_time < 2.0: + print(f" PASS: compute_fov() is fast (<2ms)") + else: + print(f" WARNING: compute_fov() took {avg_time:.3f}ms (expected <2ms)") + # Not a hard failure, just a warning + + print("\n" + "=" * 60) + print("All tests PASSED") + print("=" * 60) + sys.exit(0) + +# Initialize and run +mcrfpy.createScene("init") +mcrfpy.setScene("init") +mcrfpy.setTimer("test", run_test, 100)