Perf: edit pipeline node for zero-remap picks (Step 2, issue #34)#37
Perf: edit pipeline node for zero-remap picks (Step 2, issue #34)#37pcolt wants to merge 10 commits into
Conversation
Instead of a dedicated _edit_reader/_edit_display proxy, create a real pipeline node (kind="edit") as the edit target. All existing display, coloring, pick, and LUT logic works against it by construction — no property overrides, no cell-ID remapping needed.
…ch in surface pick - paraview_backend.py: in _pick_surface_keys_native, use edit_dataset (working_dataset) as source_dataset when in an edit session instead of a second Fetch(source) round-trip — working_dataset has the same geometry - docs/logic_flows.md: rewrite §5/5a/5b/6 to reflect the edit pipeline node approach - MIGRATION_LOG.md: add entries for Step 1 (remap caches, commit 298a3df) and Step 2 (edit pipeline node) including the rationale for choosing a real node over a dedicated reader proxy, and the future-improvement angle - CLAUDE.md: update paraview_backend.py description to match the new approach - TODO.md: remove completed performance item for issue #34
- test_edit_session.py: is_supported_dataset_type accepts only vtkUnstructuredGrid and rejects vtkPolyData, vtkStructuredGrid, and related strings - test_state_handlers.py: programmatic sync to the same node ID during an edit session must not emit a warning or snap the pipeline panel back - test_e2e_edit_selection_playwright.py: after entering edit mode, a pipeline node labelled '✏ Editing: …' must appear in the pipeline panel - test_e2e_edit_selection_playwright.py: after discarding an edit session, the original node is restored and the mesh is visible in the viewport
Surface pick performance — post-implementation findingsMarked draft after discovering surface picks remain as slow as pre-Step-1, while volume picks are now significantly faster. Investigation notes for the fix decision. What was confirmedChecked out
Step 2 is a genuine improvement for volume picks. The surface pick regression is because What B5 and B5b actually are after Step 2Both were incorrectly marked ✅ in issue #34. On every surface pick, two full Python→C++ scans still happen:
Two options for fixing B5 + B5b✅ Option 1 — Session-level caches (~40 lines,
Also removed the unreachable
Option 2 — SelectSurfaceCells on the volume mesh (~90 lines) — deferred Skip the GeometryFilter helper during edit sessions. Click precision caveat: edge/corner cells can have 2–3 boundary faces; a click returns all of them. Mitigation: project candidate faces to screen space after the dict lookup and keep only the one closest to the cursor — same
Option 2 remains the cleaner long-term architecture (eliminates the coordinate scan permanently) — tracked as a follow-up. |
set_edit_target_dataset builds _edit_source_point_indexes and _edit_boundary_elements once at session begin; both are passed through the surface-pick chain instead of rebuilding per pick. clear_edit_target_dataset clears them at session end. Also splits docs/logic_flows.md into per-domain files under docs/logic_flows/, with updated content for B5/B5b caches and new assign/materialize/discard flow detail.
_edit_target_dataset is always set at session begin; surface picks only happen in an active session. Replace unreachable Fetch branch with a defensive early return.
|
✅ - Option 1 from previous comment was implemented (fix to #34 B5 and B5b) |
SelectionTiming.emit() accepts an optional state_key so the same helper can write to different state variables. set_edit_target_dataset times the B5/B5b cache builds and exposes them via consume_session_begin_timing(). pv_begin_edit_session wraps export/begin/set_target in a SelectionTiming and emits to state.session_begin_timing_last. Both timing panels (session begin and last selection) are shown at the bottom of the edit tab.
|
Benchmark measurements (baseline vs. this PR) are tracked in the issue: issue #34 — benchmark comment. Covers session begin, volume picks, surface picks, and the net trade-off across N picks per session. |
ParaView proxy node IDs contain a colon (e.g. source:4385). Using the ID verbatim produced _edit_temp_source:4385.vtu, which the Docker deployment filesystem rejects with EPERM/Permission denied. Replace ':' with '_' before building the temp path.
- fix: check file existence after writer.Write() to catch cases where VTK reports success but no file was created (e.g. permission denied on the data directory mount)
1 participant
Overview
Closes Step 2 of issue #34.
PR #35 (B3, B4) eliminated per-pick
Fetch()and coordinate-map rebuilds by pre-building four remap caches at session begin. That was a solid improvement but still required coordinate-based cell-ID translation between the ParaView source andworking_dataseton every pick. This PR removes the remap layer entirely by making the edit source a real pipeline node that reads the same backing data as the original node — so pick cell IDs directly indexworking_datasetwith no translation needed.A dedicated reader proxy approach was explored first (branch
perf/step2-edit-reader) but dismissed: it caused regressions in coloring, LUT, and representation handling because those code paths all assume a regular pipeline node as the active source. Using a realkind="edit"node fits the existing architecture with zero property overrides and no special-casing.Beyond correctness and simplicity, the pipeline node model is also a better foundation for future improvements: real-time display updates when cell values change (update the edit node in place), and server-side field-value mutations via ParaView proxies to preserve MPI distribution without collapsing all ranks through
Fetch().Remaining bottlenecks after this PR: volume and surface picks are now fast (GPU hardware pick, no remap, session-scoped geometry caches for surface key mapping). The remaining per-session cost on large meshes is adjacency growth (B8 in issue #34).
Summary
paraview_backend.pyexport_active_dataset_for_editing(): create a realkind="edit"pipeline node (label"✏ Editing: {original}", iconmdi-pencil-outline); hide all other pipeline nodes; return the fetched dataset foredit_session.begin()_create_edit_source(): root reader →OpenDataFile(same file), no temp write; filter node → write temp VTU thenOpenDataFilecan_edit_active_node(): cheapGetDataInformationprobe replaces the expensive export-and-discard probe inupdate_ui_state()set_edit_target_dataset(): build session-scoped caches_edit_source_point_indexes(coordinate index, Perf: improve edit session performance on large meshes (3-step plan) #34 B5 fix) and_edit_boundary_elements(boundary face map, Perf: improve edit session performance on large meshes (3-step plan) #34 B5b fix) once at session begin; passed through the surface-pick chain instead of rebuilding per pickclear_edit_target_dataset(): delete edit node, restore pre-edit node visibility, clean up temp file; clears both geometry caches_pick_surface_keys_native(): use_edit_target_datasetassource_dataset; removed unreachableFetch(source)fallback (surface picks only happen in an active session where_edit_target_datasetis always set)_edit_source_dataset,_edit_source_point_indexes,_edit_target_cell_map,_edit_point_id_map) and five remap methods_active_kind_label(): return"Edit Session"forkind="edit"nodesedit_session.py: addis_supported_dataset_type()static method for cheap type probeparaview_runtime.py:update_ui_state()usescan_edit_active_node()instead of the old export probeparaview_controllers.py:pv_begin_edit_session/pv_discard_edit_sessioncallupdate_paraview_ui_state()to keep the pipeline panel in sync with the edit node;set_edit_target_dataset()call drops the removedsource_datasetparamparaview_filter_catalog.py:pipeline_icon()returnsmdi-pencil-outlineforkind="edit"nodesstate_handlers.py:on_active_pipeline_item_changeallows programmatic sync when the new value equals the current active node (suppresses spurious warning whenupdate_ui_statesyncs the panel to the edit node)docs/logic_flows/(split from monolithicdocs/logic_flows.md): updated §5/5a/5b for edit node flow and B5/B5b cache fields; new §5d/5e for assign/materialize detail and discard flow; §6c corrected for session-scoped cachesCLAUDE.md,TODO.md,MIGRATION_LOG.md: updated to reflect completed issue Perf: improve edit session performance on large meshes (3-step plan) #34test_edit_session.py:is_supported_dataset_typeaccepts onlyvtkUnstructuredGridtest_state_handlers.py: programmatic sync to the same node ID during an edit session does not warn or snap backtest_e2e_edit_selection_playwright.py: after entering edit mode, a"✏ Editing: …"node appears in the pipeline paneltest_e2e_edit_selection_playwright.py: after discarding, the original node is restored and the mesh is visibleTest plan
conda run -n coral-paraview pytest -q tests/ --ignore-glob=tests/test_e2e*.pyconda run -n coral-paraview pytest -q tests/test_e2e_edit_selection_playwright.pyKnown limitations / future work
Re-entering edit mode after each commit (B11)
After a commit, the edit node is deleted and the result is loaded as a new pipeline node. If the user wants to assign another field value, they must re-enter edit mode, which triggers a full session begin:
export_active_dataset_for_editing(Fetch + possible temp file write) +EditSession.begin()(DeepCopy) +set_edit_target_dataset(point-index and boundary-element cache builds). On large meshes this is the dominant latency for multi-field workflows.A natural fix is to keep the edit node and its caches alive after commit when only field values changed (no topology mutations from
materialize_surface_selection). The commit result would be written to disk, a new pipeline node loaded, and the edit session re-entered immediately on that new node — making subsequent field assignments instant. This requires changes to the commit lifecycle and session-node handoff and is tracked separately.Tracked in: issue #39.