Add room-aware ambient daylight through exterior openings - #204
Add room-aware ambient daylight through exterior openings#204TomasTriska88 wants to merge 2 commits into
Conversation
|
@TomasTriska88 can you add screenshots of the before and after tested on the dev container? I just want to make it easier to imagine. Thanks! |
|
I run a fork and tested this against current It's still clean against latest The fail-dark path holds on a real plan. I built it into my fork's card and ran it on a live The lit case checks out too. Rather than fake
and the ramp between them is smooth and monotonic: −6° → 0, −3° → 0.156, 0° → 0.5, +3° → 0.844, One gap while I was in there. Your existing tests pin One note on testability, since it cost me a while: the feature needs One structural thought, aimed at the card rather than at this PR. Right now every watched entity is |
Rebased onto upstream v1.5.4. Includes review follow-ups: strict fail-dark handling for non-numeric sun elevation and central sun entity tracking via collectWatchedEntities.
9ef85ee to
dc5cdd9
Compare
|
@TomasTriska88 I meant some screenshots on the PR's description. hehe |
|
Done 🙂 I added the before/after screenshots directly to the PR description, captured from the repository's Home Assistant dev container on the same seeded plan. Direct sunlight is disabled; the real I also folded in the other review feedback while rebasing onto current v1.5.4 ( The upstream |
nicosandller
left a comment
There was a problem hiding this comment.
This is high-quality work — easily the most carefully built PR in the queue right now. Pure geometry module separated from rendering, 1205 tests green, typecheck and build clean, opt-in and off by default, README plus a docs/ page, and before/after captures. The fail-dark handling for a missing sun.sun is the right instinct.
I verified it myself. Tests and typecheck pass, and the layer clips correctly — here's an amplified difference map between the flag off and on, which shows the wash landing inside each Area polygon and not leaking through the wall between the two rooms:
ambient daylight difference map
That's the clipping doing its job. Two things to sort out first, then some smaller notes.
1. At the default strength you can barely see it
That diff map is amplified 14x. The real numbers on the demo plan at noon (elevation 42.5°) are:
- max change on any pixel: 9 / 255
- average change where anything changed at all: 3.8 / 255
Side by side at normal size, off vs on:
The PR deliberately keeps strength, spread, tint and blur as internal defaults — "rather than exposing unstable calibration knobs". I understand the reasoning, but the result is a feature someone turns on, sees almost nothing, and turns back off, with no knob to reach for. Over an Area that already has a colour fill it's swamped.
Please either raise the default meaningfully or expose ambientDaylightStrength. I'd lean toward exposing it — one number is not much surface, and it's the difference between the feature being usable and not.
2. The geometry is recomputed on every render
renderAmbientDaylightLayer calls ambientOpeningSources(...) each time, which walks every opening against every area against every vertex, plus point-in-polygon probes for the inward normal. None of that changes unless the config changes — but it re-runs on every hass update, so on a busy plan it runs several times a second.
The early return means there's no cost when the flag is off, which is the important half. But it's worth caching the sources per config. (Same note I left on #212 — it's becoming a pattern worth being careful about.)
3. Smaller things
- The module docstring is now wrong.
ambient-daylight.tsopens with "This module is deliberately pure and not wired into the card yet" — it is wired in, by this PR. - A comment got mangled in
editor-forms.ts. The rewrite left a dangling fragment: "…all of the direct-sun state goes — ambientDaylight deliberately survives because it is a sibling layer. these keys is read only while the light is on…". The original sentence ended "every one of these keys is read only while…". - Patch edges are straight diagonal cuts. Visible in the diff map at the room corners. The blur softens them but the trapezoid is still geometric. Fine for V1, worth knowing.
- Toggle ordering. "Ambient daylight" sits above "Let the sun in" in the panel. Direct sunlight is the better-known feature; I'd put ambient second.
4. One thing that connects to #215
ambientOpeningTransmission decides glass as glazed ?? type === "window" — so a glazed door passes ambient light. That's the sensible rule, and it's the same question I raised on #215. We now have three separate implementations of "is this opening transparent": openingSunFraction, this one, and glowClearFraction in #215. They don't all agree.
Nothing for you to fix here — your version is the one I'd keep. But whichever of these lands first should probably pull the rule into one shared helper the others call. Worth a quick chat before either merges.
On the known limitation
Classifying a one-sided opening as exterior when a neighbouring room simply has no Area drawn is a real trap, and documenting it rather than guessing is the right call for V1. Fine by me.
Get (1) and (2) sorted plus the two comment fixes and I'm happy with this.
Generated by Claude Code
Motivation
Direct sunlight models the sun's actual rays, but it does not represent diffuse sky light entering a room when the sun is not shining directly through an opening. The most obvious case is a north-facing window: the room is still daylight-lit, even though it may receive no direct sunbeam.
What this does
ambientDaylight: truecard setting. The default isfalse, so existing plans do not change.Area; openings between two mapped Areas are treated as interior and do not become fake outdoor light sources.sunlight: falseopt-out.Config
Areas are required because V1 uses Area adjacency both to determine which side of an opening is indoors and to provide the hard clipping boundary.
Screenshots
Captured from the repository Home Assistant dev container on one deterministic two-room scene. Direct sunlight and sun dimming are off, and the real
sun.sunis pinned to daytime (elevation62.95°). The only config change between captures isambientDaylight: false→true.ambientDaylight: falseambientDaylight: trueThe left room has north and west exterior windows. With ambient daylight enabled, broad diffuse daylight enters from both openings while the adjacent room without an exterior opening stays unchanged. Capture provenance (before config, after config).
Validation
The branch is rebased and squashed to one feature commit on current
main/ v1.5.4 (288146b):null,""," ", arrays, unavailable values and non-finite elevations are regression-covered and fail darksun.sunis registered through the centralcollectWatchedEntitiespath rather than a second card-level watcherThe upstream
Validateworkflow for the current head is presentlyaction_requiredbefore any job is created (fork-run approval gate), so there is no current CI test failure to report or hide behind an older run.Tests cover default/backward compatibility, north-facing openings, interior-opening rejection, opaque/closed opening behavior, shutters, night/fail-dark behavior, bounded multi-source composition, exact Area clipping, CSS renderer ownership, unique SVG ids, editor/config round-trip and invalid/incomplete configuration.
Known limitation
Exterior-vs-interior inference assumes complete Area topology. If a real adjacent room is not represented by an Area polygon, a one-sided opening at that boundary can be classified as exterior. V1 documents this requirement rather than guessing missing building topology.