Skip to content

fix: tributaries now end at the confluence hex, not one hex before it - #26

Merged
countercheck merged 2 commits into
masterfrom
claude/fix-rivers-petering-out
May 14, 2026
Merged

fix: tributaries now end at the confluence hex, not one hex before it#26
countercheck merged 2 commits into
masterfrom
claude/fix-rivers-petering-out

Conversation

@countercheck

Copy link
Copy Markdown
Owner

What changed

_split_at_confluences in hydrology.py was trimming lower-flow tributaries with path[:cut] where cut = i — the index of the first claimed (confluence) hex. This stopped the tributary at path[i-1], one hex before the confluence, so the tributary's endpoint was never on the main trunk's polyline. The result was tributaries that appeared to dead-end on flat inland terrain.

Root cause: off-by-one in the cut index.

Fix: cut = i + 1 — the confluence hex is now included as the tributary's final point. It's already part of the trunk path, so the two SVG polylines share a vertex and visually connect. The hex is double-drawn in SVG but invisible at normal stroke widths.

Scale of impact on the sample world (128×128): 60 out of 115 rivers were ending on interior flat-land hexes before this fix.

Files changed

File Change
worldgen/stages/hydrology.py cut = icut = i + 1; updated docstring
tests/test_hydrology.py Updated test_no_shared_hexes_between_rivers to permit confluence hexes (tributary endpoints) to appear in two rivers — that sharing is now intentional

Reviewer notes

  • test_rivers_reach_ocean passes without logic changes. With the fix, a tributary mouth IS the confluence hex; the test's at_confluence check correctly identifies the next downstream trunk hex as a qualifying neighbor.
  • test_no_shared_hexes_between_rivers now excludes tributary endpoints from the "shared hex" check. Any hex shared between rivers that is not a tributary endpoint still fails the assertion, so genuine trunk duplication is still caught.
  • The yaml import error in test_cli.py / test_config.py was a pre-existing venv gap (pyyaml not installed); installing it unblocked the pre-commit hook. All 210 tests pass.

_split_at_confluences was using `cut = i` when trimming a lower-flow
tributary, stopping the path at path[i-1] — the hex immediately before
the confluence.  Because that endpoint was not on the main trunk's
polyline, tributaries appeared to peter out mid-map rather than flowing
into the main river.

Change to `cut = i + 1` so the confluence hex is included as the
tributary's final point.  It is already part of the trunk's path, so
the two polylines share a vertex and visually connect.  The shared hex
is double-drawn in SVG but is invisible at normal stroke widths.

Update test_no_shared_hexes_between_rivers to allow confluence hexes
(tributary endpoints) to appear in multiple rivers, since that sharing
is now intentional.  Trunk duplication elsewhere is still caught.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an off-by-one error in hydrology river splitting so tributaries terminate on the confluence hex (shared with the higher-flow trunk), preventing tributaries from visually “dead-ending” one hex short in rendered outputs.

Changes:

  • Include the confluence hex when trimming tributary paths in _split_at_confluences (cut = i + 1).
  • Update hydrology docstring/comments to reflect confluence-inclusive trimming semantics.
  • Adjust test_no_shared_hexes_between_rivers to allow intentional sharing at confluence endpoints.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
worldgen/stages/hydrology.py Fixes tributary trimming to include the confluence hex so tributaries visually connect to trunks.
tests/test_hydrology.py Updates shared-hex assertion logic to permit confluence endpoint sharing by design.
Comments suppressed due to low confidence (1)

worldgen/stages/hydrology.py:751

  • The comment about recalculating flow_volume refers to the “last exclusive land hex”, but trimmed paths can now intentionally include the confluence hex (a land hex that is shared with the trunk). Please update this comment to match the new trimming semantics (e.g., last land hex in the trimmed path).
                cut = i + 1  # include confluence hex so tributary visually meets the trunk
                break
        trimmed = path[:cut]
        if len(trimmed) >= 2:
            # Recalculate flow_volume from the last exclusive land hex in the trimmed path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread worldgen/stages/hydrology.py Outdated
Comment on lines +720 to +724
"""Trim each River path to end at the confluence hex.

Higher-flow rivers process first and claim their land hexes. Each subsequent
river is cut at the first land hex already owned by a higher-flow river, converting
source-to-sea duplicates into distinct source-to-confluence segments. Rivers that
shrink below 2 hexes are dropped. Original list order is preserved in the output.
river is cut at the first land hex already owned by a higher-flow river; the
confluence hex itself is included as the tributary's endpoint so that it visually
@countercheck

Copy link
Copy Markdown
Owner Author

@copilot apply changes based on the comments in this thread

Copilot AI commented May 14, 2026

Copy link
Copy Markdown
Contributor

@copilot apply changes based on the comments in this thread

Applied the review-thread updates in commit 97dca06: _split_at_confluences docs/comments now explicitly state that only intersecting tributaries are trimmed to include the confluence, while non-intersecting trunk rivers keep full paths. No UI changes in this PR, so there’s no UI screenshot to provide.

@countercheck
countercheck merged commit 502258f into master May 14, 2026
3 checks passed
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.

3 participants