Fix 'rooms explored' statistic undercounting and minimap not appearing on new maps#356
Conversation
…y generated rooms Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/fddd4ef0-c05f-48cb-8f68-8ea7779e130b Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes undercounting of the “rooms explored” statistic when rooms are generated ahead-of-time by RoomPreloader, by tracking “freshly generated” rooms in Map and consuming that flag when the player actually enters the room.
Changes:
- Added
Map._freshlyGeneratedRoomsplusconsumeIsNewRoom(x, y)to atomically mark/consume newly generated rooms. - Updated
WorldScreenroom-loading logic to increment stats based onconsumeIsNewRoom, including suppressing stat updates for non-player transitions. - Added unit tests covering the new-room flag behavior and the “rooms explored” increment rules (including preloaded rooms).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/world/map.py |
Tracks freshly generated room coordinates and exposes consumeIsNewRoom() for atomic “first entry” detection. |
src/screen/worldScreen.py |
Uses consumeIsNewRoom() to drive stat increments consistently (including preloaded rooms) and adds updateStats control for non-player transitions. |
tests/world/test_roomsExplored.py |
New tests for end-to-end “rooms explored” increment semantics (new, re-entry, preloaded, disk-loaded). |
tests/world/test_map.py |
Adds focused tests for consumeIsNewRoom() behavior (true once, false thereafter, not set for addRoom). |
CHANGELOG.md |
Documents the bug root cause, fix approach, and adds a Learning Log entry. |
| increment `rooms explored`; this handles both the pre-loaded (RoomPreloader background | ||
| thread) and direct-generate paths uniformly. Pass `updateStats=False` when calling | ||
| `_loadOrGenerateRoom` for non-player transitions (e.g., living-entity cross-room moves) | ||
| so the flag is not consumed and stats are not affected. |
There was a problem hiding this comment.
The Learning Log entry added here is marked [not yet integrated]. If this behavior is now part of the expected project conventions, please also integrate it into .github/copilot-instructions.md (and flip the tag to [integrated]), or clarify why it should remain unintegrated.
| so the flag is not consumed and stats are not affected. | |
| so the flag is not consumed and stats are not affected. This remains not yet integrated | |
| because it is a localized implementation detail for `Map`/`WorldScreen` room-generation | |
| and stats behavior, not a repository-wide contributor convention for | |
| `.github/copilot-instructions.md`. |
| import pytest | ||
|
|
There was a problem hiding this comment.
pytest is imported but never used in this test module. Please remove the unused import to keep the test file clean (and to avoid autoflake/linters flagging it).
| import pytest |
| import json | ||
| import os | ||
|
|
There was a problem hiding this comment.
json and os are imported inside this test but never used. Please remove these unused imports (autoflake will likely strip them anyway).
| import json | |
| import os |
| try: | ||
| gameMap.generateNewRoom(0, 0) | ||
| preloader.preloadNearbyRooms(0, 0, gameMap) | ||
| preloader.shutdown(wait=True) |
There was a problem hiding this comment.
This test calls preloader.shutdown(wait=True) both inside the try block and again in finally. The first call is redundant; consider keeping only the finally shutdown (or using a single shutdown after preloadNearbyRooms) to avoid double work and to make the intent clearer.
| preloader.shutdown(wait=True) |
| from lib.graphik.src.graphik import Graphik | ||
| from world.room import Room |
There was a problem hiding this comment.
These local imports duplicate the module-level imports at the top of the file (Graphik, Room). Please remove the redundant imports here to keep the tests consistent and avoid unnecessary clutter.
| from lib.graphik.src.graphik import Graphik | |
| from world.room import Room |
|
@copilot You are a coding agent tasked with resolving all open comments and review threads Instructions1. Discover all comments
2. For each comment, do the following:
3. Types of issues to handle:
4. Do NOT:
After making changes
Context
|
…own, clarify changelog Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/58f28541-b329-46c7-9ba1-ca24ed3c0973 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
All review threads addressed in 2cc8208:
All 436 tests still pass. |
|
@copilot The minimap is not showing up consistently anymore, especially with new maps. Resolve. |
…nimap on new maps Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/15f3d783-a181-4c68-995b-346849d6e690 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Fixed in 735811f. Root cause: Fix: Added |
incrementRoomsExplored()was never called for rooms pre-loaded byRoomPreloaderbecause_loadOrGenerateRoomonly incremented on thegenerateNewRoombranch — which is skipped when the room is already in memory. Result: the stat stalled in single digits regardless of how far the player explored.Additionally, the minimap would not appear consistently on new maps due to a silent error in
MapImageGenerator.Approach
Rooms explored stat fix
Introduce a "freshly generated" flag on
Mapthat decouples when a room is created from when the player enters it.Map._freshlyGeneratedRooms— thread-safeset(under existing_lock) marking rooms created fresh (not loaded from disk)Map.generateNewRoom()— adds(x, y)to the set when actually generating (double-checked, inside lock)Map.consumeIsNewRoom(x, y) -> bool— atomically checks and clears the flag; returnsTrueexactly once per newly generated room_loadOrGenerateRoomnow callsconsumeIsNewRoominstead of branching on whethergenerateNewRoomwas called inline:This handles all cases uniformly:
RoomPreloader→ flagged → consumed on player entry → increments ✓Other stat-related fixes:
initialize(): replaced unconditionalincrementRoomsExplored()withconsumeIsNewRoom(0, 0)to prevent double-counting when the player returns to the spawn room_loadOrGenerateRoom(..., updateStats=False)so they don't consume the flag or affect statsMinimap fix
MapImageGenerator.getRoomImages()andclearRoomImages()both calledos.listdir()on theroompngsdirectory without checking if it existed. There is a race between the_doSave()background thread (which triggersupdateMapImage()after each room-change save) and the main thread'sdraw()call (which creates theroompngsdirectory lazily viasaveCurrentRoomAsPNG()). When the background thread won the race,os.listdir()raisedFileNotFoundError, which was caught silently — preventingmapImage.pngfrom ever being written and leaving the minimap invisible.MapImageGenerator.getRoomImages(): returns[]immediately if theroompngsdirectory does not existMapImageGenerator.clearRoomImages(): exits early (no-op) if theroompngsdirectory does not existThis ensures
generate()never throws on a fresh game, allowingmapImage.pngto be written (initially blank) on the first map update call, so the minimap widget appears immediately and fills in as rooms are visited.