Evaluate visibility for scenes with no FDS simulation - #41
Conversation
read_fds_data() couples three separable things: the sampling grid, the extinction field, and the obstructions. Only the field has to come from FDS, but requiring it means any caller with a geometry and no fire -- a clear-air evacuation model, a unit test -- has to approximate this library instead of using it. Downstream that has produced several partial reimplementations of the ray casting and the Jin relation, each free to drift. set_grid() defines the sampling grid, extent and cell size the way read_fds_data() derives them from a slice, and allocates the obstruction array so add_visual_obstruction() is usable immediately after -- the same contract read_fds_data() offers by calling build_obstructions_array() last. set_uniform_extco() supplies one extinction coefficient everywhere in place of the slice; 0 is clear air, for which visibility is max_vis wherever a sign is in line of sight and inside its readable half-plane. get_extco_array_at_time() consults the synthetic field before the slice and its error now names both routes. build_obstructions_array() sizes itself from the grid rather than from get_extco_array_at_time(0): the obstruction map is a property of the geometry, and a scene may have no extinction data to size it from. get_visibility_to_wp() omitted the view-angle factor that get_vismap() applies, so it reported a directional sign as fully legible to a viewer standing behind it while wp_is_visible() reported the opposite. Both now form the same product. This changes the value returned for viewers outside a sign's half-plane, which was previously wrong rather than merely different; omni-directional signs (alpha=None) are unaffected. The existing suite asserts only 0 <= visibility < 100 for that accessor and so could not detect the omission. tests/test_synthetic_scene.py adds 18 tests: grid and cell-size construction, obstruction blocking by ray casting, S = C/K with the max_vis clamp, contrast scaling, and agreement between the two accessors on both sides of a directional sign.
chraibi
changed the title
There was a problem hiding this comment.
Pull request overview
This PR decouples visibility evaluation from requiring an FDS slice by adding a “synthetic scene” route (manual grid + uniform extinction), enabling clear-air/geometry-only use cases while keeping ray casting, obstructions, Jin relation, and view-angle handling inside fdsvismap.
Changes:
- Added
VisMap.set_grid()andVisMap.set_uniform_extco()to support non-FDS scenes (geometry + optionally uniform smoke). - Updated
get_extco_array_at_time()andbuild_obstructions_array()to work when no FDS slice is present. - Fixed
get_visibility_to_wp()to apply the same view-angle factor asget_vismap()andwp_is_visible().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
fdsvismap/FDSVisMap.py |
Adds synthetic-scene APIs (grid + uniform extco), adjusts obstruction sizing from grid, and makes get_visibility_to_wp apply view-angle consistently. |
tests/test_synthetic_scene.py |
Adds coverage for synthetic scenes: grid/extent construction, obstruction blocking, Jin relation + max clamp, contrast scaling, and view-angle consistency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Loading a simulation now clears any synthetic field. Previously set_uniform_extco() followed by read_fds_data() left the uniform value in place and get_extco_array_at_time() returned it, silently ignoring the slice that had just been read. The reverse direction is made explicit too: set_uniform_extco() drops self.slc, so the two sources can never both apply and the accessor never has to choose between them. set_uniform_extco() also sets the evaluation times. It previously wrote only fds_time_points while compute_all() reads vismap_time_points, so a caller who set the field but not the times met an empty-time-points failure inside compute_all(). One call is now enough; passing time_points still works and set_time_points() still overrides. New annotations use tuple[...] and X | None rather than the Tuple and Optional the file uses elsewhere: current ruff flags the latter, and this keeps the diff from adding lint errors to a tree that already has 48 of them under a newer ruff than the pinned pre-commit hook.
|
Both review comments applied in Precedence. Correct: Time points. Also correct, and worse than it looks: Two tests added for these (20 in the new file, 33 in total). The On the red CI — it is not this PRBoth failures reproduce on pristine Run Tests — That line is untouched by this PR. Checked out Code Quality — pristine So CI goes green only with an fdsreader pin (or a fix for 1.11.9) and a ruff sweep, both independent of this work. Happy to open either as a separate PR — I would not fold 48 pre-existing lint fixes into this diff. |
The examples/room_fire deck the other tests already use is enough to assert it: set_uniform_extco() then read_fds_data() must return the simulation, not the uniform value. Reverting the one-line clear in read_fds_data() fails this test, so it pins the behaviour rather than merely describing it. Closes the coverage gap noted when the precedence fix went in.
|
You are right —
Mutation-checked: reverting the one-line clear in |
The cell size is derived from the first spacing and the index snapping in _add_visual_object assumes it is positive and constant, so a descending axis gave a negative cell size and a non-uniform one an inconsistent size -- both accepted silently, both placing obstructions on the wrong cells. The docstring already required ascending input; now the code does too. Also documents the ordering contract on set_grid: walls are added after the grid, because build_obstructions_array() rebuilds from obstructions_collection and erases manual obstructions.
Quantified: what this PR changes for FDS scenesAsked whether the fixes disturb FDS-path behaviour, so I measured it on Unchanged, verified:
Changed, by design —
Omni-directional signs are bit-identical. For directional signs the returned value now honours the readable half-plane and the cos θ falloff — i.e. it now agrees with what If any existing consumer wants the old question answered, the compatible resolution is a keyword — |
A uniform field is identical at every time, so _check_time_in_computed_range rejecting t past the single computed point protected nothing and forced every caller of the synthetic route to clamp query times itself -- the downstream wrapper had already grown exactly that workaround. Static fields now resolve any time to the nearest computed point; slice-backed scenes keep the check, where a time past the simulation would silently reuse the last frame and lie. Closes #45.
…es (#95) * refactor: get clear-air visibility from fdsvismap, not from five copies fdsvismap could not evaluate a scene without an FDS run, so every caller needing visibility in clear air wrote its own approximation. There were five: OccludingVisMap, VisMapClearAir twice, ClearAirVisMap, and the ClearAirVisibility added last week. Each reimplemented the Jin relation and some of the ray casting, none applied the view angle, and they were free to drift from the library and from each other. FireDynamics/fdsvismap#41 adds set_grid() and set_uniform_extco(), so the grid and the extinction field no longer have to come from FDS. VisibilityModel.clear_air() uses them: obstructions are rasterised from the walkable polygon and emitted as row runs through the public add_visual_obstruction(), and the model wraps the resulting VisMap, which already exposes the wp_is_visible signature. pyproject pins the branch until it is released. Resolution is now an explicit parameter rather than an assumption. A cell blocks sight when its centre is outside the walkable area, so a wall thinner than one cell disappears -- the same property an FDS mesh has. At the 0.5 m default the 0.4 m walls of assets/blind_spawn_discovery stop occluding anything, which would have made that asset's premise test pass while testing nothing; the tests that depend on occlusion now ask for 0.25 m and say why. A uniform field is time-invariant, so one time point is computed and queries resolve to it. Without that the engine's first query at t > 0 was rejected as outside the computed range. test_exit_visibility_alpha reached into the surrogate's _signs and view_angle; it now asserts the same guarantee through node_is_visible. Behaviour is unchanged where it can be compared: the animation script reproduces its previous run exactly -- same learning events, same nodes, same six switches. * fix: refresh the fdsvismap pin and guard cell_size_m The lock had frozen the branch at its first commit, before the review fixes -- it worked here only because clear_air calls set_time_points explicitly and never hits the field-precedence path, but anyone syncing this branch built the unfixed library. Now locked at 7076df4, which also brings set_grid's ascending/uniform validation. clear_air(cell_size_m<=0) previously died inside set_grid with 'got 0 x coordinates', naming the symptom instead of the cause; refuse it at the boundary. * refactor: drop the static-time clamp; fdsvismap owns it now The clamp existed because a synthetic scene computed one time point and fdsvismap rejected queries past it. That is fixed at the source (FireDynamics/fdsvismap#45, commit 64d9aa7): a uniform field resolves any query time to the nearest computed point, so the wrapper no longer second-guesses the clock and clear_air instances carry one attribute fewer. Lock moved to 64d9aa7. * fix: allow direct references while fdsvismap is pinned to a branch hatchling rejects git URLs in project.dependencies without tool.hatch.metadata.allow-direct-references, so CI could not even build the package with the pin in place. Local runs missed it because the editable rebuild was masked by an unrelated build-cache failure. Remove the flag together with the pin once fdsvismap cuts a release. * fix: address review on the clear-air model Three reviewer points, all applied: clear_air on a domain smaller than two cells per axis now raises a ValueError naming the bounds and the cell size, instead of failing inside fdsvismap with "got 1 x coordinates" -- symptom, not cause. _vis is typed as a _VisBackend Protocol (wp_is_visible) rather than _VisMapCache, since clear_air assigns a live VisMap. Both backends satisfy it structurally; the annotation stops lying. fdsvismap is pinned to the exact commit instead of the branch name, so installs that bypass uv.lock (pip install .) are reproducible too. Moving the pin is now an intentional edit here, not a side effect of a push there.
Why
read_fds_data()couples three separable things: the sampling grid, theextinction field, and the obstructions. Only the field has to come from
FDS. Requiring all three means any caller that has a geometry but no fire — a
clear-air evacuation model, a unit test — cannot use this library at all and
ends up approximating it.
That has already happened. In pyFDS-Evac there are currently three partial
reimplementations of the ray casting plus the Jin relation (
OccludingVisMap,VisMapClearAir,ClearAirVisibility), each free to drift from what thislibrary actually computes, and none of them applying the view angle. This PR
removes the reason they exist.
What
set_grid— grid,extentandcell_sizederived exactly asread_fds_dataderives them from a slice. It also allocatesobstructions_array, soadd_visual_obstructionworks straight after,mirroring the contract
read_fds_dataoffers by callingbuild_obstructions_array()last. RaisesValueErroron fewer than twocoordinates per axis, since a cell size cannot be derived from one.
set_uniform_extco— one extinction coefficient in place of the slice.Rejects negative values.
get_extco_array_at_timeconsults the synthetic field first; its errormessage now names both routes instead of only
read_fds_data().build_obstructions_arraysizes from the grid rather than fromget_extco_array_at_time(0). The obstruction map is a property of thegeometry, and a scene may have no extinction data to size it from.
Everything downstream is untouched: ray casting, view angle, the
max_visclamp,
get_visibility_to_wp.Behaviour change:
get_visibility_to_wpnow applies the view angleFound while testing the above, and fixed here because it makes the float
accessor trustworthy:
So the float accessor ignored sign orientation while the boolean one honoured
it. With
alpha=90, a viewer standing west of the sign gotwp_is_visible() == Falsebutget_visibility_to_wp() == 30.0, i.e. "fullylegible". The two now form the same product and agree.
This changes returned values for viewers outside a sign's half-plane — where
the previous value was wrong rather than merely different. Omni-directional
signs (
alpha=None) are unaffected, as are all existing tests.Tests
The existing suite asserts only
0 <= visibility < 100for that accessor, so itcould not have caught the omission.
tests/test_synthetic_scene.pyadds 18:set_gridit (a centred sign is reached over a low wall from either side, and the test
would pass without the ray casting doing anything)
S = C/Kexact at K = 0.1/0.3/1.0/3.0 with themax_visclampreflecting one (C=3)
Verified the view-angle tests actually bite: reverting that one line fails 3 of
them. 31 tests pass in total (13 existing + 18 new);
ruffclean;mypycleanunder the pre-commit flags (
--disallow-untyped-defs --ignore-missing-imports --no-warn-return-any).