Skip to content

fix(engine): add_location defaults discovered=True so runtime-named places stay on the Atlas (#261/#371 follow-up) - #650

Merged
100yenadmin merged 1 commit into
mainfrom
fix/add-location-discovered-default-408
Jun 5, 2026
Merged

fix(engine): add_location defaults discovered=True so runtime-named places stay on the Atlas (#261/#371 follow-up)#650
100yenadmin merged 1 commit into
mainfrom
fix/add-location-discovered-default-408

Conversation

@100yenadmin

Copy link
Copy Markdown
Member

TL;DR

Revives PR #408 (verified absent from main). A real regression the additive discovered field (PR #371 / #261) introduced on the runtime path: add_location never set discovered, so a place the DM names into the world mid-play serialized discovered=False and the Atlas hid it until the party visited it. Pre-#371 those places were visible. This restores that with discovered: bool = True as the default, keeps deliberate fog-of-war expressible via discovered=False, and adds a 4-test regression suite.

The bug (verified, file:line anchored)

  • servers/engine/models.py:841Location.discovered: bool = False (added by fix(openworlds): atlas-seed — #261 #371 so fog-of-war seeds must opt in).
  • servers/engine/server.py (add_location) — the new-Location constructor was Location(name=…, description=…, hex=coords, region=region, travel_times=tt) with no discovered= → every runtime-added place serialized discovered=False.
  • viewer/server.py (_atlas_visible_location_ids) — a place shows when visited OR discovered is True OR discovered is None; discovered is False and not visitedhidden. Old snapshots (field absent → None) stay visible, so the regression bites only the runtime add_location path, where the model default makes it an explicit False.

Net: a far-off / future-destination place the DM adds (without make_current) is invisible on the Atlas until visited — a silent change from pre-#371 behavior.

Why discovered=True is the right default (not fog-of-war)

Signal Implication
seed_world sets day-1 regions discovered=True "known" == True
Pre-#371, add_location'd places were visible restoring, not inventing
The model already has a separate hidden flag for deliberate suppression discovered was never the runtime "hide it" lever
The False default exists so fog-of-war seeds opt in the runtime path was simply missed when the field landed

A place the DM names into the world mid-play is, by that act, known — the player should see it. Deliberate fog-of-war stays available via discovered=False.

The fix (additive, engine-only)

  • add_location gains a discovered: bool = True param, threaded into the new-Location constructor only.
  • Update path (location_id reuse) is untouched → an existing place's discovered is preserved, never clobbered by the default (so editing a hidden place's description doesn't reveal it).
  • make_current=True still arrives the party (visited=True) → visible regardless; unchanged.
  • Docstring documents the default + the discovered=False rumour path + the hidden-vs-discovered distinction.

Tests — servers/engine/tests/test_add_location_discovered.py (4)

  1. default add_location → discovered=True and visited=False (proves visibility comes from the flag, not a visit)
  2. discovered=False → fog-of-war opt-in preserved
  3. update path (location_id reuse, default discovered=True) → existing discovered=False preserved, description updated
  4. make_current=Truevisited=True + becomes current_location_id

Locally: 4/4 new tests pass; the 16 existing discovered/seed_world content tests stay green.

Closes #408.

…laces stay on the Atlas (#261/#371 follow-up)

PR #371 added `discovered: bool = False` to the Location model (fog-of-war
seeds opt IN) but the runtime path `add_location` was never updated — so a
place the DM names into the world mid-play serialized discovered=False and the
Atlas hid it until the party visited it. Pre-#371 those places were visible.

This threads a `discovered: bool = True` param into the new-Location
constructor only (the update path preserves an existing place's discovered
state), keeps deliberate fog-of-war expressible via discovered=False, and adds
a 4-test regression suite.

Closes #408.
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@100yenadmin, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 25 minutes and 44 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9908884f-cb05-4e90-9c4d-62a10f2bc981

📥 Commits

Reviewing files that changed from the base of the PR and between 1a604fd and 41d8545.

📒 Files selected for processing (2)
  • servers/engine/server.py
  • servers/engine/tests/test_add_location_discovered.py

Comment @coderabbitai help to get the list of available commands and usage tips.

@100yenadmin
100yenadmin merged commit 003b1b3 into main Jun 5, 2026
14 checks passed
@100yenadmin
100yenadmin deleted the fix/add-location-discovered-default-408 branch June 5, 2026 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant