Add falling pixel snow over the landing mountain - #380
Closed
TheGreatAxios wants to merge 2 commits into
Closed
Conversation
TheGreatAxios
force-pushed
the
cl-5605-mountain-snow-v2
branch
from
August 8, 2026 18:58
a54235d to
111f196
Compare
Critique review caught that the fade-out phase thinned the mountain silhouette toward empty while the snowfield kept animating at full density, so the decoration outlasted the mark it was drifting over.
This was referenced Aug 8, 2026
TheGreatAxios
added a commit
that referenced
this pull request
Aug 9, 2026
* Make the landing snow actually render on an idle screen Three independent gates kept the falling-snow decoration over the landing mountain from ever drawing: the mount-time paint was still=true, which the old snowOn check tied directly to snow visibility; paintLanding early-returned whenever it wasn't re-entered with animating=true, which never happens while idle; and its only caller was the turn monitor, which deliberately stops ticking once idle. Fix: snow visibility no longer depends on `still` (that flag now only freezes the mountain's own draw/fill/fade timeline, not the flakes over it), the early-return is gone so idle repaints actually happen, and the shell's createAppShell now repaints the landing off the renderer's own FRAME event whenever the landing is up and not mid-turn-animation. That event is already scoped to shell lifetime (wired at construction, unwired in dispose) and paintLanding already no-ops once the landing tears down, so this needed no new timer to arm or leak, and it doesn't touch the turn monitor's cadence at all. The alternative — a separate timer armed from createLandingAbove and stopped in the landing teardown — was rejected: it would duplicate the monitor's own cadence-management responsibility for no real gain, since the FRAME event already has the right lifetime. Added a real-mount-path regression test in landing.test.ts that goes through createAppShell and lets the renderer's FRAME event drive the repaint, unlike every existing test in that file, which drives the mark by calling paintLanding directly with a hand-picked clock. That gap is exactly why this shipped broken and nobody caught it. This supersedes PR #380, which added the snow-drawing code but never made it reachable; that PR should stay open until this one is reviewed and can then be closed in favor of this one. * Throttle the idle landing repaint to ~8fps FRAME fires from inside the on-demand render loop, and paintLanding reassigns a fresh StyledText to every row every call regardless of whether content changed, which unconditionally dirties renderables and requests another render. Left unthrottled that turned the idle landing into a perpetual ~60fps render loop instead of riding an existing one. Guard the onFrame-driven repaint with a stored last-paint timestamp so it only actually repaints once every ~125ms, and drop the comment that falsely claimed only changed rows get touched. Also thread an explicit reducedMotion input through the mark renderer (renderMark/markChunks/paintLandingMark/paintLanding), separate from still, so a future reduced-motion setting has a real plumbing path instead of overloading still (which only freezes the mountain's own draw/fill/fade timeline). Defaults to false everywhere; no behavior change until something sets it. Updates the mark-anim.ts and landing.ts docblocks that stated still suppressed snow, which PR #428 made no longer true. * Drive the idle landing repaint off a mount-scoped timer, not a throttled FRAME hook The FRAME-driven throttle added in the previous commit killed the landing's self-driving loop entirely: the renderer only keeps rendering because each paint dirties a row, which schedules the next FRAME; skipping a paint on a throttled tick breaks that chain on the very next frame and the snow freezes after the first paint. Any throttle above zero frames has the same effect, since the throttle and the frame source were the same mechanism. Replace it with a plain ~125ms interval armed when the shell mounts (the landing exists for the lifetime of the shell until the first transcript row tears it down) and cleared on whichever teardown happens first: the landing going away, or the shell disposing. The timer self-cancels once the renderer reports destroyed, so headless test harnesses that skip explicit shell.dispose() don't leave it firing against torn-down renderables. Also replace the test's manual renderOnce-loop clock with a real wall-clock wait and no frame pumping at all, so the test can no longer stay green while production's self-driving mechanism is dead — that blind spot is exactly how the frozen throttle shipped in the first place.
Collaborator
Author
|
Superseded by #428, which is merged. This PR added the snow drawing code. It could never render: three independent conditions each suppressed the draw, and nothing drove frames on an idle landing, so the feature was unreachable by construction. Every test for it passed because they called the paint function directly with flags the product never produces. #428 keeps this drawing code, fixes all three gates, and gives the landing a real clock scoped to its own lifetime. Its test waits on wall-clock time with no manual render calls and fails on a build where the snow is frozen — the property every previous test lacked. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Approach
renderMark: deterministic column seeds, ~18% of columns host one flake, ~0.55 rows/s fall with slight per-column speed variation·inUI.textFaintonly on zero-coverage sky cells (never on silhouette or unrevealed mountain cells)renderMarkvia the existing 250ms clock — no landing changes neededTest plan
bun run typecheckbun test src/tui-opentui/mark-anim.test.ts src/tui-opentui/landing.test.tsFixes CL-5605