From 054dd023a7fb5bb1d571b07f74cbcf18e0176914 Mon Sep 17 00:00:00 2001 From: Laura Kasian Date: Mon, 28 Sep 2026 21:08:14 -0700 Subject: [PATCH 1/7] feat: server-owned variable records --- .../models/common/visor_variable_record.py | 69 +++ .../models/common/visor_variable_state.py | 6 +- .../models/persist/persisted_viewer_state.py | 8 +- .../requests/visor_save_state_response.py | 42 +- .../models/runtime/scene/runtime_app_state.py | 7 +- .../runtime/scene/runtime_scene_state.py | 5 +- .../models/runtime/visor_scene_details.py | 4 + src/ansys/visor/viewer/vtk/scene/base.py | 60 ++- .../viewer/vtk/scene/visor_state_mapper.py | 12 +- .../vtk/variables/visor_variable_aggregate.py | 153 +++++++ tests/integration/test_save_load_state.py | 62 +++ tests/integration/test_visor_file_io.py | 2 +- tests/unit/app/test_visor_vtk_local.py | 4 +- tests/unit/models/test_runtime_scene_state.py | 32 +- .../models/test_visor_save_state_response.py | 58 ++- .../unit/models/test_visor_variable_record.py | 87 ++++ tests/unit/vtk/io/test_visor_file_io.py | 2 +- tests/unit/vtk/scene/test_base.py | 418 ++++++++++++++++-- .../unit/vtk/scene/test_visor_state_mapper.py | 57 ++- tests/unit/vtk/test_wire_format_identity.py | 96 ++++ .../test_visor_variable_aggregate.py | 106 +++++ .../test_visor_variable_empty_block.py | 72 +++ 22 files changed, 1264 insertions(+), 98 deletions(-) create mode 100644 src/ansys/visor/viewer/models/common/visor_variable_record.py create mode 100644 src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py create mode 100644 tests/unit/models/test_visor_variable_record.py create mode 100644 tests/unit/vtk/variables/test_visor_variable_aggregate.py create mode 100644 tests/unit/vtk/variables/test_visor_variable_empty_block.py diff --git a/src/ansys/visor/viewer/models/common/visor_variable_record.py b/src/ansys/visor/viewer/models/common/visor_variable_record.py new file mode 100644 index 00000000..ca188fdc --- /dev/null +++ b/src/ansys/visor/viewer/models/common/visor_variable_record.py @@ -0,0 +1,69 @@ +"""Server-owned record for one color variable across every dataset in the scene.""" + +from typing import Dict, List, Tuple + +from pydantic import BaseModel, ConfigDict, Field, field_serializer + +from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType +from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState +from ansys.visor.viewer.models.persist.scene.persisted_scene_state import _IDENTIFIER_SEPARATOR + + +def compose_variable_identifier(type: VisorVtkVariableType, name: str, num_components: int) -> str: + """Compose the stable variable identifier ``::::``. + + The inverse of :func:`derive_variable_fields_from_identifier`, and the same + composition the client uses, so identifiers in existing files key identically. + """ + return f"{type.value}{_IDENTIFIER_SEPARATOR}{name}{_IDENTIFIER_SEPARATOR}{num_components}" + + +class VisorVariableRecord(BaseModel): + """ + The server's record of one variable, keyed by its composed identifier. + + Every field is required: a record never carries an unset range. ``magnitude_range`` + and ``ranges`` are the custom (effective) ranges; ``default_magnitude_range`` and + ``default_ranges`` are the ranges widened across every participating part. + ``part_ids`` lists the parts that carry the variable. + + The identity and range fields share their attribute names and aliases with + :class:`VisorVariableState`, which is the persisted projection (:meth:`to_variable_state`). + """ + model_config = ConfigDict(populate_by_name=True) + + id: str = Field(...) + array_name: str = Field(..., alias="arrayName") + type: VisorVtkVariableType = Field(...) + num_components: int = Field(..., alias="numComponents") + part_ids: List[int] = Field(..., alias="partIds") + default_magnitude_range: Tuple[float, float] = Field(..., alias="defaultMagnitudeRange") + default_ranges: List[Tuple[float, float]] = Field(..., alias="defaultRanges") + magnitude_range: Tuple[float, float] = Field(..., alias="magnitudeRange") + ranges: List[Tuple[float, float]] = Field(...) + + @field_serializer("type") + def _serialize_type(self, value: VisorVtkVariableType) -> str: + """Emit the wire value (e.g. "POINT"/"CELL") for both dict-mode and JSON-mode dumps.""" + return value.value + + def to_variable_state(self) -> VisorVariableState: + """Project the record onto the persisted entry: identity plus the effective ranges.""" + return VisorVariableState( + id=self.id, + array_name=self.array_name, + type=self.type, + num_components=self.num_components, + magnitude_range=self.magnitude_range, + ranges=list(self.ranges), + ) + + +class VisorVariableRecords(BaseModel): + """Holder for the scene's variable records, keyed by identifier. + + Created once by the scene and never rebound. Writers assign a new dict to + ``variables`` (copy-on-write); readers are handed ``model_copy(deep=True)``. + """ + variables: Dict[str, VisorVariableRecord] = Field(default_factory=dict) + diff --git a/src/ansys/visor/viewer/models/common/visor_variable_state.py b/src/ansys/visor/viewer/models/common/visor_variable_state.py index 335a23b2..489d4775 100644 --- a/src/ansys/visor/viewer/models/common/visor_variable_state.py +++ b/src/ansys/visor/viewer/models/common/visor_variable_state.py @@ -21,8 +21,10 @@ class VisorVariableState(BaseModel): explicitly (rather than only opaquely inside ``id``) so a consumer can act on them without parsing the client-built identifier. - In the future, the backend can own this, but for now we can treat this as passthrough data, - as the id value is stable across sessions. + This is the persisted entry: the identity plus the effective ranges at save time. The backend owns + the variables as :class:`VisorVariableRecord` (``visor_variable_record.py``), and this model is that + record's persisted projection (``VisorVariableRecord.to_variable_state``). The id value is stable + across sessions. """ model_config = ConfigDict(populate_by_name=True) diff --git a/src/ansys/visor/viewer/models/persist/persisted_viewer_state.py b/src/ansys/visor/viewer/models/persist/persisted_viewer_state.py index 16161113..9d77413e 100644 --- a/src/ansys/visor/viewer/models/persist/persisted_viewer_state.py +++ b/src/ansys/visor/viewer/models/persist/persisted_viewer_state.py @@ -41,7 +41,8 @@ def from_components(cls, datasets: Dict[str, PersistedDatasetState], camera: VisorCameraState | None = None, cross_section: VisorCrossSectionState | None = None, - variable_states: Dict[str, "VisorVariableState"] | None = None, + *, + variable_states: Dict[str, "VisorVariableState"], ) -> "PersistedViewerStateV1": """ Create a PersistedViewerStateV1 instance from UI settings and dataset states. @@ -51,7 +52,8 @@ def from_components(cls, unit (str | None): Scene unit (or None). datasets (Dict[str, PersistedDatasetState]): Dataset states keyed by dataset name. camera (VisorCameraState | None): Camera state (or None). - variable_states (Dict[str, VisorVariableState] | None): Variable states keyed by variable identifier (or None). + variable_states (Dict[str, VisorVariableState]): Variable states keyed by variable identifier. + Keyword-only and required; pass ``{}`` for none. Returns: PersistedViewerStateV1: The constructed viewer state. """ @@ -64,6 +66,6 @@ def from_components(cls, edges_enabled=edges_enabled, bounding_box_enabled=bounding_box_enabled, dataset_states=datasets, - variable_states=variable_states or {}, + variable_states=variable_states, ) return cls(ui=ui_state, scene=scene_state) diff --git a/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py b/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py index a3929015..79f05614 100644 --- a/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py +++ b/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py @@ -1,16 +1,22 @@ import json from typing import Any -from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator +from ansys.visor.viewer.core.visor_logging import VisorDefaultLogger from ansys.visor.viewer.models.runtime.scene.runtime_app_state import RuntimeAppState +logger = VisorDefaultLogger(__name__) + class VisorSaveStateResponse(BaseModel): """ Model for the response from the client to receive the runtime viewer state, which is in turn used by the server to save the persisted state of the Visor viewer. + + The server owns the variable records, so any ``appState.scene.variableStates`` the + browser sends is discarded before validation (see :meth:`_discard_browser_variable_states`). """ model_config = ConfigDict(arbitrary_types_allowed=True, populate_by_name=True) @@ -18,6 +24,40 @@ class VisorSaveStateResponse(BaseModel): request_id: int = Field(alias="requestId") app_state: RuntimeAppState = Field(alias="appState") + @model_validator(mode="before") + @classmethod + def _discard_browser_variable_states(cls, data: Any) -> Any: + """Drop ``appState.scene.variableStates`` from the browser's reply. + + The browser's entries lack the record's default fields and are not the + authority; ``get_state`` takes the variables from the server's record. + """ + if not isinstance(data, dict): + return data + key = "appState" if "appState" in data else ("app_state" if "app_state" in data else None) + if key is None: + return data + app_state = data[key] + if isinstance(app_state, str): + try: + app_state = json.loads(app_state) + except (TypeError, ValueError): + return data + if not isinstance(app_state, dict): + return data + scene = app_state.get("scene") + if not isinstance(scene, dict): + return data + discarded = False + for scene_key in ("variableStates", "variable_states"): + if scene_key in scene: + discarded = True + if not discarded: + return data + scene = {k: v for k, v in scene.items() if k not in ("variableStates", "variable_states")} + logger.debug("browser variableStates discarded") + return {**data, key: {**app_state, "scene": scene}} + @field_validator("app_state", mode="before") @classmethod def _coerce_app_state(cls, v: Any) -> Any: diff --git a/src/ansys/visor/viewer/models/runtime/scene/runtime_app_state.py b/src/ansys/visor/viewer/models/runtime/scene/runtime_app_state.py index d1df1823..af5f88b3 100644 --- a/src/ansys/visor/viewer/models/runtime/scene/runtime_app_state.py +++ b/src/ansys/visor/viewer/models/runtime/scene/runtime_app_state.py @@ -7,7 +7,7 @@ from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.common.visor_cross_section_state import VisorCrossSectionState from ansys.visor.viewer.models.common.visor_ui_state import VisorUIState -from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState +from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecord from ansys.visor.viewer.models.runtime.dataset.runtime_dataset_state import RuntimeDatasetState from ansys.visor.viewer.models.runtime.scene.runtime_scene_state import RuntimeSceneState @@ -39,7 +39,8 @@ def from_components( bounding_box_enabled: bool | None = None, cross_section: VisorCrossSectionState | None = None, camera: VisorCameraState | None = None, - variable_states: Dict[str, VisorVariableState] | None = None, + *, + variable_states: Dict[str, VisorVariableRecord], ) -> "RuntimeAppState": """Construct a RuntimeAppState from the given components. @@ -56,7 +57,7 @@ def from_components( edges_enabled=edges_enabled, bounding_box_enabled=bounding_box_enabled, dataset_states=dataset_states, - variable_states=variable_states or {}, + variable_states=variable_states, ) return cls( ui=ui, diff --git a/src/ansys/visor/viewer/models/runtime/scene/runtime_scene_state.py b/src/ansys/visor/viewer/models/runtime/scene/runtime_scene_state.py index cefaabcc..32188726 100644 --- a/src/ansys/visor/viewer/models/runtime/scene/runtime_scene_state.py +++ b/src/ansys/visor/viewer/models/runtime/scene/runtime_scene_state.py @@ -6,7 +6,7 @@ from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.common.visor_cross_section_state import VisorCrossSectionState -from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState +from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecord from ansys.visor.viewer.models.runtime.dataset.runtime_dataset_state import RuntimeDatasetState @@ -28,7 +28,8 @@ class RuntimeSceneState(BaseModel): edges_enabled: bool | None = Field(default=None, alias="edgesEnabled") bounding_box_enabled: bool | None = Field(default=None, alias="boundingBoxEnabled") dataset_states: Dict[int, "RuntimeDatasetState"] = Field(default_factory=dict, alias="datasetStates") - variable_states: Dict[str, "VisorVariableState"] = Field(default_factory=dict, alias="variableStates") + # The server's variable records (see VisorSceneBase._variable_records); never taken from the browser. + variable_states: Dict[str, "VisorVariableRecord"] = Field(default_factory=dict, alias="variableStates") @field_validator("dataset_states", mode="before") @classmethod diff --git a/src/ansys/visor/viewer/models/runtime/visor_scene_details.py b/src/ansys/visor/viewer/models/runtime/visor_scene_details.py index 3a34acc8..9fab9687 100644 --- a/src/ansys/visor/viewer/models/runtime/visor_scene_details.py +++ b/src/ansys/visor/viewer/models/runtime/visor_scene_details.py @@ -5,6 +5,7 @@ from pydantic import BaseModel, ConfigDict, Field from ansys.visor.viewer.models.common.visor_ui_state import VisorUIState +from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecord from ansys.visor.viewer.models.runtime.dataset.runtime_dataset_state import RuntimeDatasetState from ansys.visor.viewer.models.runtime.scene.runtime_app_state import RuntimeAppState from ansys.visor.viewer.models.runtime.vtk.renderer_annotation import RendererAnnotation @@ -36,6 +37,8 @@ def from_components( cross_section_enabled: bool | None = None, edges_enabled: bool | None = None, bounding_box_enabled: bool | None = None, + *, + variable_states: Dict[str, VisorVariableRecord], ) -> "VisorSceneDetails": """Construct an instance from components. @@ -55,6 +58,7 @@ def from_components( cross_section_enabled=cross_section_enabled, edges_enabled=edges_enabled, bounding_box_enabled=bounding_box_enabled, + variable_states=variable_states, ) return cls( app_state=app_state, diff --git a/src/ansys/visor/viewer/vtk/scene/base.py b/src/ansys/visor/viewer/vtk/scene/base.py index 2651240a..0853d347 100644 --- a/src/ansys/visor/viewer/vtk/scene/base.py +++ b/src/ansys/visor/viewer/vtk/scene/base.py @@ -3,7 +3,7 @@ import json import threading from abc import ABC, abstractmethod -from typing import TYPE_CHECKING, List +from typing import TYPE_CHECKING, Dict, List from trame_server import Server @@ -15,6 +15,8 @@ from ansys.visor.viewer.core.visor_types import VisorDatasetType from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.common.visor_ui_state import VisorUIState +from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecords +from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState from ansys.visor.viewer.models.persist.persisted_viewer_state import PersistedViewerStateV1 from ansys.visor.viewer.models.runtime.visor_scene_details import VisorSceneDetails from ansys.visor.viewer.renderer.base import IRenderer @@ -24,6 +26,10 @@ from ansys.visor.viewer.vtk.scene.visor_state_mapper import VisorStateMapper from ansys.visor.viewer.vtk.scene_graph import VisorSceneGraph from ansys.visor.viewer.vtk.variables.visor_part_variables import VisorPartVariables +from ansys.visor.viewer.vtk.variables.visor_variable_aggregate import ( + build_variable_records, + overlay_persisted_ranges, +) from ansys.visor.viewer.vtk.variables.visor_variable_update import VisorVariableUpdate if TYPE_CHECKING: @@ -69,6 +75,7 @@ class VisorSceneBase(ABC): _edges_enabled: bool _bounding_box_enabled: bool _ui_state: VisorUIState + _variable_records: VisorVariableRecords def __init__( self, @@ -102,6 +109,13 @@ def __init__( panel_top_right_tab_index=0, ) + # The server's variable records, keyed by composed identifier. Created once + # here and never rebound. Written only by _rebuild_variable_records, under + # _vtk_lock, by assigning a new dict (copy-on-write) so the unlocked reader in + # get_scene_details never sees a dict change size. Handed out only as + # model_copy(deep=True): a shallow copy would share the dict and its entries. + self._variable_records = VisorVariableRecords() + # Server-tracked widget toggles. Absolute values, never toggles. # Initialised to the client widgets' own constructor defaults so a # get_state before the client has ever spoken reports what the client @@ -261,6 +275,9 @@ async def get_state(self, timeout: float) -> PersistedViewerStateV1: runtime_state.scene.edges_enabled = self._edges_enabled runtime_state.scene.bounding_box_enabled = self._bounding_box_enabled runtime_state.ui = self._ui_state.model_copy() + # Variables and unit are the server's, never the browser's reply. + runtime_state.scene.variable_states = self._variable_records.model_copy(deep=True).variables + runtime_state.scene.unit = self._dataset_registry.unit runtime_state.scene.dataset_states = registry_dataset_states persisted = self._state_mapper.runtime_to_persisted(runtime_state) @@ -283,12 +300,17 @@ def apply_state(self, state: PersistedViewerStateV1): # Transform the frontend PersistedViewerStateV1 -> RuntimeAppState runtime_app_state = self._state_mapper.persisted_to_runtime(state) + # Variable records: rebuilt from the registry and overlaid with the file's + # ranges, then placed on the runtime state (CC-1), all before + # _restore_part_states, which reads each colored part's range from that dict. + self._rebuild_variable_records("load", persisted=state.scene.variable_states) + runtime_app_state.scene.variable_states = self._variable_records.model_copy(deep=True).variables + # One call per state class: updates the server's stored state and its VTK objects. self._restore_part_states(runtime_app_state) self._restore_widget_state(runtime_app_state) self._restore_ui_state(runtime_app_state) self._restore_camera_state(runtime_app_state) - # TODO: restore variable states when they are synced back to the server. # Finalize here, not on the load path. On a cold load -- viewer started with no dataset, # then a state loaded -- load_state adds the datasets and only then calls apply_state, so a @@ -337,6 +359,10 @@ def get_scene_details(self) -> VisorSceneDetails: The UI record is passed as a copy and never as the instance, for the reason :meth:`get_state` gives: a panel trigger landing after this call would otherwise mutate a payload already served. + + The variable records are passed as a deep copy of the holder, also + without the lock: writers rebind ``variables`` to a new dict, so this + read sees either the old dict or the new one, never one in flux. """ if self._scene_graph is None: self._initialize_scene_graph() @@ -355,6 +381,7 @@ def get_scene_details(self) -> VisorSceneDetails: cross_section_enabled=self._cross_section_enabled, edges_enabled=self._edges_enabled, bounding_box_enabled=self._bounding_box_enabled, + variable_states=self._variable_records.model_copy(deep=True).variables, ) def get_scene_details_json(self) -> str: @@ -375,6 +402,7 @@ def clear(self): self._renderer.deregister_all() self._dataset_registry.clear() self._scene_graph = None + self._rebuild_variable_records("clear") def populate_scene(self): """Update widgets to reflect the current scene contents. @@ -460,6 +488,7 @@ def add_dataset(self, input: VisorDatasetType, metadata: ExtendedMetadata) -> in node_ids = [leaf.id for leaf in part_nodes] self._dataset_registry.add(dataset_id, dataset_name, input, node_ids, metadata) + self._rebuild_variable_records("add") return dataset_id @@ -491,6 +520,7 @@ def remove_dataset(self, dataset_id: int): # Unregister the dataset self._dataset_registry.remove(dataset_id) + self._rebuild_variable_records("remove") def list_variables_for_dataset(self, dataset_id: int) -> List[VisorPartVariables]: return self._dataset_registry.list_variables(dataset_id) @@ -515,11 +545,37 @@ def update_variables_for_dataset(self, dataset_id: int, variables: List[VisorVar with timer.phase("update_descendant_parts"): dataset_node.refresh_descendant_variable_metadata(include_self=True) + # After the reload and the metadata refresh: a width change changes the id. + self._rebuild_variable_records("update_variables") + with timer.phase("render"): self.render() timer.log() + def _rebuild_variable_records( + self, + point: str, + persisted: Dict[str, VisorVariableState] | None = None, + ) -> None: + """Rebuild the variable records from the registry. + + ``persisted`` is ``None`` at add, remove, clear and update: the previous + records are carried by the custom-range rule. On load it is the file's + ``variable_states``: the records are built fresh and the file's ranges + overlaid. + + Under ``_vtk_lock``. Assigns a new dict to the holder (copy-on-write); + never mutates the live dict or its entries. + """ + with self._vtk_lock: + if persisted is None: + records = build_variable_records(self._dataset_registry, self._variable_records.variables) + else: + records = overlay_persisted_ranges(build_variable_records(self._dataset_registry, {}), persisted) + self._variable_records.variables = records + logger.debug("variable records rebuilt at %s: %d records", point, len(records)) + def render(self): """Delegate to the renderer backend. diff --git a/src/ansys/visor/viewer/vtk/scene/visor_state_mapper.py b/src/ansys/visor/viewer/vtk/scene/visor_state_mapper.py index cc28ecc6..9a322beb 100644 --- a/src/ansys/visor/viewer/vtk/scene/visor_state_mapper.py +++ b/src/ansys/visor/viewer/vtk/scene/visor_state_mapper.py @@ -45,8 +45,11 @@ def runtime_to_persisted(self, runtime_app_state: RuntimeAppState) -> PersistedV unit = scene_state.unit # Scene - camera camera = scene_state.camera - # Scene - variables: Pass through as-is since they are already keyed by stable variable identifier - variable_states = scene_state.variable_states + # Scene - variables: the server's records, projected onto the persisted entry + variable_states = { + variable_id: record.to_variable_state() + for variable_id, record in scene_state.variable_states.items() + } # Scene - datasets runtime_dataset_states = scene_state.dataset_states persisted_dataset_states = {} @@ -93,8 +96,9 @@ def persisted_to_runtime(self, state: PersistedViewerStateV1) -> RuntimeAppState # scene - camera camera = scene_state.camera - # scene - variables: Pass through as-is since they are already keyed by stable variable identifier - variable_states = scene_state.variable_states or {} + # scene - variables: the records are built from the datasets, not from the file. + # VisorSceneBase.apply_state overlays the file's ranges onto the rebuilt records. + variable_states = {} # scene - datasets: Convert persisted dataset states to runtime runtime_dataset_states = {} diff --git a/src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py b/src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py new file mode 100644 index 00000000..397e49e6 --- /dev/null +++ b/src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py @@ -0,0 +1,153 @@ +"""Pure aggregation of the scene's variable records from the dataset registry.""" + +from typing import TYPE_CHECKING, Dict, List, Tuple + +from ansys.visor.viewer.core.visor_logging import VisorDefaultLogger +from ansys.visor.viewer.models.common.visor_variable_record import ( + VisorVariableRecord, + compose_variable_identifier, +) +from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState + +if TYPE_CHECKING: + from ansys.visor.viewer.vtk.datasets.visor_dataset_registry import VisorDatasetRegistry + +logger = VisorDefaultLogger(__name__) + +Range = Tuple[float, float] + + +def _widen(current: Range | None, other: Range | None) -> Range | None: + """Return the union of two ranges; ``None`` contributes nothing.""" + if other is None: + return current + other = (float(other[0]), float(other[1])) + if current is None: + return other + return (min(current[0], other[0]), max(current[1], other[1])) + + +def _follow_or_keep(previous_custom: Range, previous_default: Range, new_default: Range) -> Range: + """A custom slot equal to its previous default follows the new default; otherwise it is kept.""" + if tuple(previous_custom) == tuple(previous_default): + return new_default + return tuple(previous_custom) + + +def build_variable_records( + registry: "VisorDatasetRegistry", + previous: Dict[str, VisorVariableRecord], +) -> Dict[str, VisorVariableRecord]: + """Build the records from every dataset's variables, carrying custom ranges from ``previous``. + + 1. Union: each part carrying a variable joins that identifier's ``part_ids``. + 2. Widen: default ranges are the min of the mins and the max of the maxes. + 3. Custom: an existing id keeps an edited custom slot; an unedited slot (equal to the + previous default) follows the new default. A new id starts with custom equal to default. + + Returns a new dict of new records; ``previous`` is never mutated. + """ + parts: Dict[str, List[int]] = {} + identity: Dict[str, tuple] = {} + magnitude: Dict[str, Range | None] = {} + components: Dict[str, List[Range | None]] = {} + + for dataset in list(registry.datasets.values()): + for part_variables in dataset.list_variables(): + for variable in part_variables.variables: + n = int(variable.num_components) + variable_id = compose_variable_identifier(variable.type, variable.name, n) + if variable_id not in identity: + identity[variable_id] = (variable.name, variable.type, n) + parts[variable_id] = [] + magnitude[variable_id] = None + components[variable_id] = [None] * n + if part_variables.part_id not in parts[variable_id]: + parts[variable_id].append(part_variables.part_id) + + variable_magnitude = variable.magnitude_range + if variable_magnitude is None and n == 1 and variable.ranges: + variable_magnitude = variable.ranges[0] + magnitude[variable_id] = _widen(magnitude[variable_id], variable_magnitude) + + slots = components[variable_id] + for k in range(min(n, len(variable.ranges))): + slots[k] = _widen(slots[k], variable.ranges[k]) + + records: Dict[str, VisorVariableRecord] = {} + for variable_id, (name, variable_type, n) in identity.items(): + default_magnitude = magnitude[variable_id] or (0.0, 0.0) + default_ranges = [slot if slot is not None else (0.0, 0.0) for slot in components[variable_id]] + + custom_magnitude = default_magnitude + custom_ranges = list(default_ranges) + old = previous.get(variable_id) + if old is not None: + custom_magnitude = _follow_or_keep(old.magnitude_range, old.default_magnitude_range, default_magnitude) + custom_ranges = [ + _follow_or_keep(old.ranges[k], old.default_ranges[k], default_ranges[k]) + if k < len(old.ranges) and k < len(old.default_ranges) else default_ranges[k] + for k in range(n) + ] + + records[variable_id] = VisorVariableRecord( + id=variable_id, + array_name=name, + type=variable_type, + num_components=n, + part_ids=sorted(parts[variable_id]), + default_magnitude_range=default_magnitude, + default_ranges=default_ranges, + magnitude_range=custom_magnitude, + ranges=custom_ranges, + ) + return records + + +def overlay_persisted_ranges( + records: Dict[str, VisorVariableRecord], + persisted: Dict[str, VisorVariableState], +) -> Dict[str, VisorVariableRecord]: + """Overlay the file's custom ranges onto freshly built records. + + - A null or absent ``magnitudeRange`` falls back to the default (DEBUG). + - ``ranges`` whose length differs from ``num_components`` fall back to the default (DEBUG). + - A file id with no record is dropped (WARNING). + + Returns a new dict; ``records`` is never mutated. + """ + result = dict(records) + for variable_id, entry in (persisted or {}).items(): + record = result.get(variable_id) + if record is None: + logger.warning("persisted variable %s has no record in the scene; dropped", variable_id) + continue + + magnitude = entry.magnitude_range + if magnitude is None: + logger.debug("persisted variable %s has no magnitudeRange; default used", variable_id) + magnitude = record.default_magnitude_range + + ranges = entry.ranges + if ranges is None or len(ranges) != record.num_components: + logger.debug( + "persisted variable %s has %s ranges for %d components; default used", + variable_id, None if ranges is None else len(ranges), record.num_components, + ) + ranges = record.default_ranges + + result[variable_id] = record.model_copy( + update={"magnitude_range": tuple(magnitude), "ranges": [tuple(r) for r in ranges]}, + deep=True, + ) + return result + + +def resolve_record_range(record: VisorVariableRecord, component: int) -> Range | None: + """Return the effective range for a component slot: ``-1`` is magnitude, ``0..n-1`` a component.""" + if component == -1: + return record.magnitude_range + if 0 <= component < record.num_components and component < len(record.ranges): + return record.ranges[component] + return None + diff --git a/tests/integration/test_save_load_state.py b/tests/integration/test_save_load_state.py index 157d2214..cde56c51 100644 --- a/tests/integration/test_save_load_state.py +++ b/tests/integration/test_save_load_state.py @@ -43,9 +43,11 @@ from ansys.visor.viewer.app.visor_vtk import VisorVTK from ansys.visor.viewer.core.metadata import ExtendedMetadata +from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType from ansys.visor.viewer.models.common.part_properties import PartProperties from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.common.visor_ui_state import VisorUIState +from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState from ansys.visor.viewer.models.persist.dataset.persisted_dataset_state import PersistedDatasetState from ansys.visor.viewer.models.persist.persisted_viewer_state import PersistedViewerStateV1 from ansys.visor.viewer.models.runtime.dataset.runtime_dataset_state import ( @@ -276,6 +278,7 @@ def _make_state(self, dataset_states: dict, unit: str = "m") -> PersistedViewerS edges_enabled=None, bounding_box_enabled=None, datasets=dataset_states, + variable_states={}, ) def test_single_dataset_is_restored_into_empty_scene(self, file_io, iface, tmp_path): @@ -458,6 +461,7 @@ def test_saved_visor_json_carries_registry_part_state_keyed_by_name(self, iface, }, ) }, + variable_states={}, ) async def _frontend_round_trip(timeout: float = 5.0): @@ -507,6 +511,7 @@ def test_reloading_the_saved_state_restores_the_registry(self, file_io, iface, t }, ) }, + variable_states={}, ) file_io.write_state(str(tmp_path), state) @@ -529,6 +534,62 @@ def test_reloading_the_saved_state_restores_the_registry(self, file_io, iface, t assert record.variable_id == "POINT::pressure::1" assert record.variable_component == 0 + def test_reloading_a_colored_part_restores_its_file_range(self, file_io, iface, tmp_path): + """I-1: a colored part loads at the range the file stores, through the server's record. + + plate.vtp carries a 2-component point array "UV" whose component-0 data + range is (-110.0, 110.0). The file stores a custom range that differs + from it in both ends, so a pass proves the file's range reached the + mapper through the rebuilt, overlaid record (CC-1) and not the default. + """ + original = file_to_dataset(_vtp_path()) + snap = file_io.get_persisted_dataset_path(str(tmp_path), "plate") + file_io.write_dataset(snap, original) + + state = PersistedViewerStateV1.from_components( + ui_state=VisorUIState(), + unit="m", + orthographic_enabled=None, + cross_section_enabled=None, + edges_enabled=None, + bounding_box_enabled=None, + datasets={ + "plate": PersistedDatasetState( + serialized_dataset_path=str(snap), + parts={ + "plate": PartProperties(color_by="POINT::UV::2", color_by_component=0) + }, + ) + }, + variable_states={ + "POINT::UV::2": VisorVariableState( + id="POINT::UV::2", + array_name="UV", + type=VisorVtkVariableType.POINT, + num_components=2, + magnitude_range=(1.0, 2.0), + ranges=[(-50.0, 60.0), (-7.0, 8.0)], + ) + }, + ) + file_io.write_state(str(tmp_path), state) + + iface._scene._push_runtime_state = MagicMock() + iface._scene._renderer.flush_wasm_state = MagicMock() + + iface.load_state(str(tmp_path)) + + dataset = next(iter(iface._scene.datasets.values())) + part_id = dataset.part_index.part_ids[0] + mapper = iface._scene._renderer._pipelines[part_id].mapper + assert mapper.GetArrayName() == "UV" + assert mapper.GetScalarRange() == pytest.approx((-50.0, 60.0)) + + held = iface._scene._variable_records.variables["POINT::UV::2"] + assert held.ranges == [(-50.0, 60.0), (-7.0, 8.0)] + assert held.magnitude_range == (1.0, 2.0) + assert held.default_ranges[0] == (-110.0, 110.0) + def test_saved_visor_json_carries_the_camera_record_not_the_browsers(self, iface, tmp_path): """save_state writes the server's camera record, not the browser's reply. @@ -551,6 +612,7 @@ def test_saved_visor_json_carries_the_camera_record_not_the_browsers(self, iface unit="m", dataset_states={}, camera=_reply_camera(), + variable_states={}, ) async def _frontend_round_trip(timeout: float = 5.0): diff --git a/tests/integration/test_visor_file_io.py b/tests/integration/test_visor_file_io.py index f315627e..62268ca9 100644 --- a/tests/integration/test_visor_file_io.py +++ b/tests/integration/test_visor_file_io.py @@ -347,7 +347,7 @@ def test_vtkhdf_multiblock_snapshot_can_be_deleted_after_load(self, file_io, tmp state = PersistedViewerStateV1.from_components( ui_state=VisorUIState(), unit="m", orthographic_enabled=None, cross_section_enabled=None, edges_enabled=None, bounding_box_enabled=None, - datasets={} + datasets={}, variable_states={} ) deleted = file_io.delete_stale_snapshots(str(tmp_path), state) diff --git a/tests/unit/app/test_visor_vtk_local.py b/tests/unit/app/test_visor_vtk_local.py index 4ac73634..5e39298c 100644 --- a/tests/unit/app/test_visor_vtk_local.py +++ b/tests/unit/app/test_visor_vtk_local.py @@ -407,7 +407,7 @@ def _make_state_with_datasets(snapshot_path: str | None, name="model", unit="m") return PersistedViewerStateV1.from_components( ui_state=VisorUIState(), unit=unit, orthographic_enabled=None, cross_section_enabled=None, edges_enabled=None, bounding_box_enabled=None, - datasets={name: ds_state} + datasets={name: ds_state}, variable_states={} ) @@ -519,7 +519,7 @@ def test_load_state_passes_correct_metadata_to_add_dataset(tmp_path, iface): state = PersistedViewerStateV1.from_components( ui_state=VisorUIState(), unit="mm", orthographic_enabled=None, cross_section_enabled=None, edges_enabled=None, bounding_box_enabled=None, - datasets={"mesh": ds_state} + datasets={"mesh": ds_state}, variable_states={} ) built_meta = MagicMock(spec=ExtendedMetadata) diff --git a/tests/unit/models/test_runtime_scene_state.py b/tests/unit/models/test_runtime_scene_state.py index 6b4210b5..c88dc3f9 100644 --- a/tests/unit/models/test_runtime_scene_state.py +++ b/tests/unit/models/test_runtime_scene_state.py @@ -3,9 +3,8 @@ import pytest from pydantic import ValidationError -from ansys.visor.viewer.models.common.visor_variable_state import ( - VisorVariableState, -) +from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType +from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecord from ansys.visor.viewer.models.runtime.dataset.runtime_dataset_state import ( RuntimeDatasetState, ) @@ -102,13 +101,24 @@ def test_dataset_states_accept_valid_mapping(): # ------------------------------------------------------------------ def test_variable_states_default_and_assignment(): - """variable_states should accept valid mapping.""" - - data = { - "a": MagicMock(spec=VisorVariableState), - } + """variable_states holds the server's records: a real record built from literals.""" + + record = VisorVariableRecord( + id="POINT::pressure::1", + array_name="pressure", + type=VisorVtkVariableType.POINT, + num_components=1, + part_ids=[1], + default_magnitude_range=(0.0, 10.0), + default_ranges=[(0.0, 10.0)], + magnitude_range=(2.0, 3.0), + ranges=[(2.0, 3.0)], + ) - state = RuntimeSceneState(variable_states=data) + state = RuntimeSceneState(variable_states={"POINT::pressure::1": record}) - assert "a" in state.variable_states - assert isinstance(state.variable_states["a"], VisorVariableState) + stored = state.variable_states["POINT::pressure::1"] + assert isinstance(stored, VisorVariableRecord) + assert stored.magnitude_range == (2.0, 3.0) + assert stored.default_ranges == [(0.0, 10.0)] + assert stored.part_ids == [1] diff --git a/tests/unit/models/test_visor_save_state_response.py b/tests/unit/models/test_visor_save_state_response.py index b5d97b41..c241e9dc 100644 --- a/tests/unit/models/test_visor_save_state_response.py +++ b/tests/unit/models/test_visor_save_state_response.py @@ -1,5 +1,5 @@ import json -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch import pytest from pydantic import ValidationError @@ -115,14 +115,11 @@ def test_model_dump_contains_fields(monkeypatch): def test_save_path_rejects_a_variable_state_missing_the_identity_fields(): - """The save path must keep raising on a client that stops emitting the fields. - - ``VisorVariableState`` is shared between ``PersistedSceneState.variable_states`` - and ``RuntimeSceneState.variable_states``, so making the three identity - fields optional on the model would have relaxed this coercion too. The - tolerance for old save files lives on the persisted container instead, and - this pins the fact that it did not leak here: reads tolerate absence, - writes do not. + """Rewritten in place (3.5.1 increment 1): the save path no longer rejects this entry. + + The server owns the variable records, so the browser's ``variableStates`` is + discarded before validation. An entry missing the identity fields (an old + client) therefore parses, and nothing of it reaches the model. """ payload = { "requestId": 1, @@ -139,16 +136,13 @@ def test_save_path_rejects_a_variable_state_missing_the_identity_fields(): }, } - with pytest.raises(ValidationError) as excinfo: - VisorSaveStateResponse.model_validate(payload) + resp = VisorSaveStateResponse.model_validate(payload) - # Reported under the wire alias, which is the spelling validation ran by. - reported = {error["loc"][-1] for error in excinfo.value.errors()} - assert {"arrayName", "type", "numComponents"} <= reported + assert resp.app_state.scene.variable_states == {} def test_save_path_accepts_a_variable_state_carrying_the_identity_fields(): - """The same payload with the three fields present validates, so the guard is specific.""" + """Rewritten in place (3.5.1 increment 1): a complete browser entry is discarded too.""" payload = { "requestId": 1, "appState": { @@ -169,7 +163,35 @@ def test_save_path_accepts_a_variable_state_carrying_the_identity_fields(): resp = VisorSaveStateResponse.model_validate(payload) - stored = resp.app_state.scene.variable_states["POINT::pressure::1"] - assert stored.array_name == "pressure" - assert stored.num_components == 1 + assert resp.app_state.scene.variable_states == {} + + +def test_save_response_discards_browser_variable_states_from_a_json_string_app_state(): + """#21: the discard also runs when appState arrives as a JSON string, and logs one DEBUG line. + + The entry lacks the record's default fields (partIds, defaultMagnitudeRange, + defaultRanges), so without the discard it would fail validation against the record. + """ + app_state = json.dumps({ + "scene": { + "unit": "mm", + "variableStates": { + "POINT::pressure::1": { + "id": "POINT::pressure::1", + "arrayName": "pressure", + "type": "POINT", + "numComponents": 1, + "magnitudeRange": [0.0, 1.0], + "ranges": [[0.0, 1.0]], + } + }, + } + }) + + with patch("ansys.visor.viewer.models.runtime.requests.visor_save_state_response.logger") as mock_logger: + resp = VisorSaveStateResponse.model_validate({"requestId": 7, "appState": app_state}) + + assert resp.app_state.scene.variable_states == {} + assert resp.app_state.scene.unit == "mm" + mock_logger.debug.assert_called_once_with("browser variableStates discarded") diff --git a/tests/unit/models/test_visor_variable_record.py b/tests/unit/models/test_visor_variable_record.py new file mode 100644 index 00000000..7992b610 --- /dev/null +++ b/tests/unit/models/test_visor_variable_record.py @@ -0,0 +1,87 @@ +"""Tests for the server-owned variable record (3.5.1 increment 1, tests 1-4). + +Every expected value is a hand-written literal. +""" +import pytest +from pydantic import ValidationError + +from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType +from ansys.visor.viewer.models.common.visor_variable_record import ( + VisorVariableRecord, + compose_variable_identifier, +) +from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState +from ansys.visor.viewer.models.persist.scene.persisted_scene_state import ( + derive_variable_fields_from_identifier, +) + + +def _record_kwargs(): + return dict( + id="CELL::temperature::1", + array_name="temperature", + type=VisorVtkVariableType.CELL, + num_components=1, + part_ids=[3, 5], + default_magnitude_range=(-1.0, 7.0), + default_ranges=[(-1.0, 7.0)], + magnitude_range=(0.5, 6.5), + ranges=[(0.5, 6.5)], + ) + + +def test_record_rejects_a_missing_default_magnitude_range(): + """#1: every field is required; a record without its default range does not validate.""" + kwargs = _record_kwargs() + del kwargs["default_magnitude_range"] + + with pytest.raises(ValidationError) as excinfo: + VisorVariableRecord(**kwargs) + + assert {error["loc"][-1] for error in excinfo.value.errors()} == {"defaultMagnitudeRange"} + + +def test_record_type_dumps_as_its_wire_value(): + """#2: the enum serializes as "CELL", in both dict and JSON dumps, under every alias.""" + record = VisorVariableRecord(**_record_kwargs()) + + assert record.model_dump(by_alias=True) == { + "id": "CELL::temperature::1", + "arrayName": "temperature", + "type": "CELL", + "numComponents": 1, + "partIds": [3, 5], + "defaultMagnitudeRange": (-1.0, 7.0), + "defaultRanges": [(-1.0, 7.0)], + "magnitudeRange": (0.5, 6.5), + "ranges": [(0.5, 6.5)], + } + assert '"type":"CELL"' in record.model_dump_json(by_alias=True) + + +def test_to_variable_state_projects_identity_and_the_effective_ranges(): + """#3: the persisted projection keeps identity and custom ranges, and drops the rest.""" + projected = VisorVariableRecord(**_record_kwargs()).to_variable_state() + + assert isinstance(projected, VisorVariableState) + assert projected.model_dump(by_alias=True) == { + "id": "CELL::temperature::1", + "arrayName": "temperature", + "type": "CELL", + "numComponents": 1, + "magnitudeRange": (0.5, 6.5), + "ranges": [(0.5, 6.5)], + } + + +def test_compose_variable_identifier_is_the_inverse_of_derive(): + """#4: composes the hand-written literal id, which derive splits back into its fields.""" + identifier = compose_variable_identifier(VisorVtkVariableType.POINT, "pressure", 1) + + assert identifier == "POINT::pressure::1" + assert derive_variable_fields_from_identifier("POINT::pressure::1") == { + "type": "POINT", + "array_name": "pressure", + "num_components": 1, + } + diff --git a/tests/unit/vtk/io/test_visor_file_io.py b/tests/unit/vtk/io/test_visor_file_io.py index fe1873ac..c4f3135b 100644 --- a/tests/unit/vtk/io/test_visor_file_io.py +++ b/tests/unit/vtk/io/test_visor_file_io.py @@ -465,7 +465,7 @@ def _make_state(self, dataset_states: dict) -> PersistedViewerStateV1: return PersistedViewerStateV1.from_components( ui_state=VisorUIState(), unit="m", orthographic_enabled=None, cross_section_enabled=None, edges_enabled=None, bounding_box_enabled=None, - datasets=dataset_states + datasets=dataset_states, variable_states={} ) def test_deletes_vtkhdf_not_in_state(self, file_io, tmp_path): diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index 3c9ad6a4..fe66afe8 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -599,7 +599,7 @@ def mocked_scene(renderer): # must return a real RuntimeAppState rather than a MagicMock (whose # dataset_states would be a MagicMock and raise on iteration). s._state_mapper.persisted_to_runtime.return_value = RuntimeAppState.from_components( - ui=VisorUIState(dark_theme=False), unit="m", dataset_states={} + ui=VisorUIState(dark_theme=False), unit="m", dataset_states={}, variable_states={} ) s._vtk_lock = _LockSpy() return s @@ -800,15 +800,20 @@ def _seed_part_variables(registry, variables, part_id=NODE_ID, dataset_id=1): ] -def _runtime_state(part_states, variable_states=None, dataset_id=1): - """A real RuntimeAppState carrying the given per-part records.""" +def _runtime_state(part_states, dataset_id=1): + """A real RuntimeAppState carrying the given per-part records. + + ``variable_states`` is empty: since 3.5.1 the mapper returns ``{}`` and + apply_state fills it from the server's rebuilt record (CC-1). A file's + variable entries are handed to :func:`_apply` instead. + """ return RuntimeAppState.from_components( ui=VisorUIState(dark_theme=False), unit="m", dataset_states={ dataset_id: RuntimeDatasetState(id=dataset_id, part_states=part_states) }, - variable_states=variable_states or {}, + variable_states={}, ) @@ -828,6 +833,7 @@ def _frontend_state(): part_states={NODE_ID: RuntimePartProperties(id=NODE_ID, opacity=0.99)}, ) }, + variable_states={}, ) @@ -848,11 +854,17 @@ def _runtime_to_persisted(runtime_state): return captured -def _apply(scene, runtime_state): - """Run apply_state with the mapper stubbed to return *runtime_state*.""" +def _apply(scene, runtime_state, file_variable_states=None): + """Run apply_state with the mapper stubbed to return *runtime_state*. + + *file_variable_states* is what the loaded file carries in + ``scene.variable_states``; apply_state overlays it onto the rebuilt record. + """ scene._state_mapper = MagicMock(name="state_mapper") scene._state_mapper.persisted_to_runtime.return_value = runtime_state - return scene.apply_state(MagicMock(name="persisted_state")) + persisted = MagicMock(name="persisted_state") + persisted.scene.variable_states = dict(file_variable_states or {}) + return scene.apply_state(persisted) class _DepthRecordingRegistry(VisorDatasetRegistry): @@ -1079,6 +1091,7 @@ async def _get_runtime_state_async(timeout): unit="m", dataset_states={}, camera=reply_camera, + variable_states={}, ) scene._get_runtime_state_async = _get_runtime_state_async @@ -1185,13 +1198,15 @@ def test_apply_state_pushes_a_json_encodable_runtime_state(scene, registry): """ pushed = {} scene._push_runtime_state = lambda state: pushed.update(state=state) + # The pushed variable entry is now the server's record, rebuilt from the registry (CC-1). + _seed_part_variables(registry, [_pressure_variable()]) _apply( scene, _runtime_state( {NODE_ID: RuntimePartProperties(id=NODE_ID, opacity=0.25, variable_id=VARIABLE_ID)}, - variable_states={VARIABLE_ID: _variable_state()}, ), + file_variable_states={VARIABLE_ID: _variable_state()}, ) encoded = json.dumps(pushed["state"].model_dump(by_alias=True)) @@ -1319,6 +1334,7 @@ def _persisted_state(camera): bounding_box_enabled=None, datasets={}, camera=camera, + variable_states={}, ) @@ -1769,18 +1785,30 @@ def test_apply_state_short_diffuse_color_is_a_logged_no_op(scene, registry, pipe # Load path — the colour-variable branch # --------------------------------------------------------------------------- -def _color_variable_state(component, **variable_kwargs): - """A part coloured by the fixture's point array, at *component*.""" +def _color_variable_state(component, variable_id=VARIABLE_ID): + """A part coloured by *variable_id*, at *component*.""" return _runtime_state( { NODE_ID: RuntimePartProperties( - id=NODE_ID, variable_id=VARIABLE_ID, variable_component=component + id=NODE_ID, variable_id=variable_id, variable_component=component ) }, - variable_states={VARIABLE_ID: _variable_state(**variable_kwargs)}, ) +def _file_entries(**variable_kwargs): + """The loaded file's ``scene.variable_states``: one entry for VARIABLE_ID.""" + return {VARIABLE_ID: _variable_state(**variable_kwargs)} + + +def _seed_parts(registry, variables_by_part, dataset_id=1): + """Give the registry's dataset per-part variable metadata for several parts.""" + registry.datasets[dataset_id].list_variables.return_value = [ + VisorPartVariables(part_id=part_id, part_name=f"part{part_id}", variables=list(variables)) + for part_id, variables in variables_by_part.items() + ] + + def _seed_unconfigured_mapper(pipeline): """Distinctive, non-default mapper state so a no-op is visible as one.""" pipeline.mapper.SetScalarVisibility(0) @@ -1793,7 +1821,7 @@ def test_apply_state_restores_the_magnitude_range_when_component_is_minus_one( """A stored component of -1 reads magnitude_range, not ranges[0].""" _seed_part_variables(registry, [_pressure_variable()]) - _apply(scene, _color_variable_state(-1)) + _apply(scene, _color_variable_state(-1), _file_entries()) assert pipeline.mapper.GetArrayName() == "pressure" assert pipeline.mapper.GetScalarRange() == pytest.approx((0.0, 49.0)) @@ -1803,7 +1831,7 @@ def test_apply_state_restores_the_per_component_range(scene, registry, pipeline) """A stored component of 0 reads ranges[0], not magnitude_range.""" _seed_part_variables(registry, [_pressure_variable()]) - _apply(scene, _color_variable_state(0)) + _apply(scene, _color_variable_state(0), _file_entries()) assert pipeline.mapper.GetArrayName() == "pressure" assert pipeline.mapper.GetScalarRange() == pytest.approx((10.0, 20.0)) @@ -1817,7 +1845,7 @@ def test_apply_state_negative_component_other_than_minus_one_is_a_logged_no_op( _seed_unconfigured_mapper(pipeline) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply(scene, _color_variable_state(-2)) + _apply(scene, _color_variable_state(-2), _file_entries()) assert mock_logger.warning.call_count == 1 assert pipeline.mapper.GetScalarVisibility() == 0 @@ -1832,7 +1860,7 @@ def test_apply_state_component_beyond_the_stored_ranges_is_a_logged_no_op( _seed_unconfigured_mapper(pipeline) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply(scene, _color_variable_state(3)) + _apply(scene, _color_variable_state(3), _file_entries()) assert mock_logger.warning.call_count == 1 assert pipeline.mapper.GetScalarVisibility() == 0 @@ -1840,64 +1868,78 @@ def test_apply_state_component_beyond_the_stored_ranges_is_a_logged_no_op( def test_apply_state_absent_range_is_a_logged_no_op(scene, registry, pipeline): - """A variable entry with no magnitude range applies nothing.""" + """Rewritten in place (3.5.1 increment 1): an absent file range now loads at the default. + + The overlay fills a null ``magnitudeRange`` from the record's default + (D3), so the entry the restore reads always carries a range: the part is + colored at the widened default (0.0, 49.0) and nothing is refused. + """ _seed_part_variables(registry, [_pressure_variable()]) _seed_unconfigured_mapper(pipeline) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply(scene, _color_variable_state(-1, magnitude_range=None)) + _apply(scene, _color_variable_state(-1), _file_entries(magnitude_range=None)) - assert mock_logger.warning.call_count == 1 - assert pipeline.mapper.GetScalarVisibility() == 0 - assert pipeline.mapper.GetScalarRange() == pytest.approx((11.0, 22.0)) + assert mock_logger.warning.call_count == 0 + assert pipeline.mapper.GetScalarVisibility() == 1 + assert pipeline.mapper.GetScalarRange() == pytest.approx((0.0, 49.0)) def test_apply_state_unknown_variable_identifier_is_a_logged_no_op( scene, registry, pipeline ): - """A stored identifier with no variable entry applies nothing.""" + """A stored identifier with no record applies nothing. + + Rewritten in place (3.5.1 increment 1): the entry now comes from the + record, which the registry's "pressure" always produces, so the part names + an identifier no dataset carries. + """ _seed_part_variables(registry, [_pressure_variable()]) _seed_unconfigured_mapper(pipeline) - runtime = _runtime_state( - { - NODE_ID: RuntimePartProperties( - id=NODE_ID, variable_id=VARIABLE_ID, variable_component=0 - ) - }, - variable_states={}, - ) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply(scene, runtime) + _apply(scene, _color_variable_state(0, variable_id="POINT::absent::1")) assert mock_logger.warning.call_count == 1 assert pipeline.mapper.GetScalarVisibility() == 0 def test_apply_state_unknown_array_name_is_a_logged_no_op(scene, registry, pipeline): - """An array the part does not carry applies nothing.""" - _seed_part_variables(registry, [_pressure_variable()]) + """An array the part does not carry applies nothing. + + Rewritten in place (3.5.1 increment 1): the record's array name comes from + the registry, so "pressure" lives on another part and this part carries + only a cell array. + """ + temperature = VisorVariable( + index=0, type=VisorVtkVariableType.CELL, name="temperature", num_components=1, + num_points=96, ranges=[(0.0, 95.0)], magnitude_range=(0.0, 95.0), + ) + _seed_parts(registry, {NODE_ID: [temperature], SECOND_NODE_ID: [_pressure_variable()]}) _seed_unconfigured_mapper(pipeline) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply(scene, _color_variable_state(0, array_name="no_such_array")) + _apply(scene, _color_variable_state(0), _file_entries()) assert mock_logger.warning.call_count == 1 assert pipeline.mapper.GetScalarVisibility() == 0 def test_apply_state_array_width_mismatch_is_a_logged_no_op(scene, registry, pipeline): - """Same name and association, different width: a different quantity.""" - _seed_part_variables(registry, [_pressure_variable()]) + """Same name and association, different width: a different quantity. + + Rewritten in place (3.5.1 increment 1): the width-3 record exists because + another part carries a width-3 "pressure"; this part's is width 1. + """ + wide_pressure = VisorVariable( + index=0, type=VisorVtkVariableType.POINT, name="pressure", num_components=3, + num_points=50, ranges=[(10.0, 20.0), (0.0, 1.0), (0.0, 2.0)], magnitude_range=(0.0, 30.0), + ) + _seed_parts(registry, {NODE_ID: [_pressure_variable()], SECOND_NODE_ID: [wide_pressure]}) _seed_unconfigured_mapper(pipeline) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply( - scene, - _color_variable_state( - 0, num_components=3, ranges=((10.0, 20.0), (0.0, 1.0), (0.0, 2.0)) - ), - ) + _apply(scene, _color_variable_state(0, variable_id="POINT::pressure::3")) assert mock_logger.warning.call_count == 1 assert pipeline.mapper.GetScalarVisibility() == 0 @@ -1922,11 +1964,10 @@ def test_apply_state_variable_id_without_component_is_a_logged_no_op( _seed_unconfigured_mapper(pipeline) runtime = _runtime_state( {NODE_ID: RuntimePartProperties(id=NODE_ID, variable_id=VARIABLE_ID)}, - variable_states={VARIABLE_ID: _variable_state()}, ) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply(scene, runtime) + _apply(scene, runtime, _file_entries()) assert mock_logger.warning.call_count == 1 assert pipeline.mapper.GetScalarVisibility() == 0 @@ -1939,11 +1980,10 @@ def test_apply_state_component_without_variable_id_does_not_clear( pipeline.mapper.SetScalarVisibility(1) runtime = _runtime_state( {NODE_ID: RuntimePartProperties(id=NODE_ID, variable_component=0)}, - variable_states={VARIABLE_ID: _variable_state()}, ) with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: - _apply(scene, runtime) + _apply(scene, runtime, _file_entries()) assert mock_logger.warning.call_count == 1 assert pipeline.mapper.GetScalarVisibility() == 1 @@ -2075,6 +2115,7 @@ async def _get_runtime_state_async(timeout): cross_section_enabled=reply_toggle, edges_enabled=reply_toggle, bounding_box_enabled=reply_toggle, + variable_states={}, ) scene._get_runtime_state_async = _get_runtime_state_async @@ -2239,6 +2280,7 @@ def _toggle_runtime_state(**toggles): unit="m", dataset_states={}, **toggles, + variable_states={}, ) @@ -2456,6 +2498,7 @@ async def _get_runtime_state_async(timeout): unit="m", dataset_states={}, orthographic_enabled=reply_orthographic, + variable_states={}, ) scene._get_runtime_state_async = _get_runtime_state_async @@ -2702,6 +2745,7 @@ async def _get_runtime_state_async(timeout): unit="m", dataset_states={}, cross_section=reply_plane, + variable_states={}, ) scene._get_runtime_state_async = _get_runtime_state_async @@ -2876,6 +2920,7 @@ def _sync(origin, normal): origin=LOADED_CROSS_SECTION_ORIGIN, normal=LOADED_CROSS_SECTION_NORMAL, ), + variable_states={}, ) ) @@ -2972,6 +3017,7 @@ async def _get_runtime_state_async(timeout): ui=reply_ui, unit="m", dataset_states={}, + variable_states={}, ) scene._get_runtime_state_async = _get_runtime_state_async @@ -2983,6 +3029,7 @@ def _panel_runtime_state(ui): ui=ui, unit="m", dataset_states={}, + variable_states={}, ) @@ -3228,3 +3275,282 @@ def test_get_state_hands_out_a_copy_of_the_ui_record(scene): assert persisted.ui is not scene._ui_state +# =========================================================================== +# 3.5.1 increment 1 -- the server's variable records +# +# Rebuild points (#10-#16), the set_state push (#18) and get_state (#19, #20). +# Expected values are hand-written literals: the fixture "pressure" variable is +# magnitude (0.0, 49.0) and component (10.0, 20.0) on NODE_ID. +# =========================================================================== + +AGGREGATE_LOGGER = "ansys.visor.viewer.vtk.variables.visor_variable_aggregate.logger" + + +def _pressure_record(**overrides): + """The record the fixture "pressure" on NODE_ID rebuilds to, with custom == default.""" + from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecord + + fields = dict( + id=VARIABLE_ID, + array_name="pressure", + type=VisorVtkVariableType.POINT, + num_components=1, + part_ids=[NODE_ID], + default_magnitude_range=(0.0, 49.0), + default_ranges=[(10.0, 20.0)], + magnitude_range=(0.0, 49.0), + ranges=[(10.0, 20.0)], + ) + fields.update(overrides) + return VisorVariableRecord(**fields) + + +def _temperature_variable() -> VisorVariable: + return VisorVariable( + index=0, type=VisorVtkVariableType.CELL, name="temperature", num_components=1, + num_points=96, ranges=[(1.0, 2.0)], magnitude_range=(1.0, 2.0), + ) + + +@pytest.fixture +def records_scene(): + """Scene with a real registry and a MagicMock scene graph and renderer.""" + s = _ConcreteScene(MagicMock(name="server"), dark_mode=False, renderer=MagicMock(name="renderer")) + s._scene_graph = MagicMock(name="scene_graph") + s._scene_graph.get_descendant_node.return_value.get_descendant_part_nodes.return_value = [] + s._dataset_registry = VisorDatasetRegistry() + return s + + +def test_rebuild_at_add_builds_the_record_after_the_registry_add(records_scene): + """#10: add_dataset rebuilds after registry.add, so the new dataset's variable is held.""" + registry = records_scene._dataset_registry + records_scene._scene_graph.load_dataset.return_value = 1 + + def _registry_add(dataset_id, name, data, node_ids, metadata): + registry.datasets[dataset_id] = _make_part_dataset( + dataset_id, [NODE_ID], + part_variables=[VisorPartVariables(NODE_ID, "part", [_pressure_variable()])], + ) + + with ( + patch.object(registry, "get_sanitized_metadata_name", return_value="ds"), + patch.object(registry, "add", side_effect=_registry_add), + ): + records_scene.add_dataset(MagicMock(name="input"), MagicMock(name="metadata")) + + assert records_scene._variable_records.variables == {VARIABLE_ID: _pressure_record()} + + +def test_rebuild_at_remove_drops_ids_with_no_remaining_part(records_scene): + """#11: removing the only dataset carrying "temperature" drops its record.""" + registry = records_scene._dataset_registry + registry.datasets = { + 1: _make_part_dataset(1, [NODE_ID], part_variables=[ + VisorPartVariables(NODE_ID, "part", [_pressure_variable()])]), + 2: _make_part_dataset(2, [SECOND_NODE_ID], part_variables=[ + VisorPartVariables(SECOND_NODE_ID, "part", [_temperature_variable()])]), + } + records_scene._rebuild_variable_records("add") + assert sorted(records_scene._variable_records.variables) == ["CELL::temperature::1", VARIABLE_ID] + + records_scene.remove_dataset(2) + + assert records_scene._variable_records.variables == {VARIABLE_ID: _pressure_record()} + + +def test_rebuild_at_clear_empties_the_records(records_scene): + """#12: clear leaves no record.""" + records_scene._dataset_registry.datasets = { + 1: _make_part_dataset(1, [NODE_ID], part_variables=[ + VisorPartVariables(NODE_ID, "part", [_pressure_variable()])]), + } + records_scene._rebuild_variable_records("add") + holder = records_scene._variable_records + + records_scene.clear() + + assert records_scene._variable_records.variables == {} + assert records_scene._variable_records is holder + + +def test_rebuild_at_update_follows_the_reload_and_the_metadata_refresh(records_scene): + """#13: a width change drops the old id and starts the new one at default. + + Call-order pin: registry.update_variables (which reloads the part variables), + then refresh_descendant_variable_metadata, then the rebuild. + """ + from ansys.visor.viewer.vtk.scene import base as base_module + + registry = records_scene._dataset_registry + dataset = _make_part_dataset(1, [NODE_ID], part_variables=[ + VisorPartVariables(NODE_ID, "part", [_pressure_variable()])]) + registry.datasets = {1: dataset} + records_scene._rebuild_variable_records("add") + + wide_pressure = VisorVariable( + index=0, type=VisorVtkVariableType.POINT, name="pressure", num_components=3, + num_points=50, ranges=[(1.0, 2.0), (3.0, 4.0), (5.0, 6.0)], magnitude_range=(0.0, 7.0), + ) + order = [] + + def _update_variables(dataset_id, variables): + order.append("update_variables") + dataset.list_variables.return_value = [VisorPartVariables(NODE_ID, "part", [wide_pressure])] + + node = records_scene._scene_graph.get_descendant_node.return_value + node.refresh_descendant_variable_metadata.side_effect = lambda **_: order.append("refresh") + real_build = base_module.build_variable_records + + def _build(*args): + order.append("rebuild") + return real_build(*args) + + with ( + patch.object(registry, "update_variables", side_effect=_update_variables), + patch.object(base_module, "build_variable_records", side_effect=_build), + ): + records_scene.update_variables_for_dataset(1, []) + + assert order == ["update_variables", "refresh", "rebuild"] + assert records_scene._variable_records.variables == { + "POINT::pressure::3": _pressure_record( + id="POINT::pressure::3", + num_components=3, + default_magnitude_range=(0.0, 7.0), + default_ranges=[(1.0, 2.0), (3.0, 4.0), (5.0, 6.0)], + magnitude_range=(0.0, 7.0), + ranges=[(1.0, 2.0), (3.0, 4.0), (5.0, 6.0)], + ) + } + + +def test_load_overlays_the_file_range_before_the_part_restore(scene, registry): + """#14: the file's range is held after load. + + Call-order pin: rebuild, overlay and the CC-1 placement all precede + _restore_part_states -- asserted by recorded order and by the entry the + restore sees at the moment it is called. + """ + from ansys.visor.viewer.vtk.scene import base as base_module + + _seed_part_variables(registry, [_pressure_variable()]) + order = [] + seen_by_restore = {} + real_build = base_module.build_variable_records + real_overlay = base_module.overlay_persisted_ranges + + def _build(*args): + order.append("rebuild") + return real_build(*args) + + def _overlay(*args): + order.append("overlay") + return real_overlay(*args) + + def _restore(runtime_app_state): + order.append("restore") + seen_by_restore["entry"] = runtime_app_state.scene.variable_states.get(VARIABLE_ID) + + with ( + patch.object(base_module, "build_variable_records", side_effect=_build), + patch.object(base_module, "overlay_persisted_ranges", side_effect=_overlay), + patch.object(scene, "_restore_part_states", side_effect=_restore), + ): + _apply( + scene, + _runtime_state({}), + _file_entries(magnitude_range=(1.0, 2.0), ranges=((3.0, 4.0),)), + ) + + expected = _pressure_record(magnitude_range=(1.0, 2.0), ranges=[(3.0, 4.0)]) + assert order == ["rebuild", "overlay", "restore"] + assert seen_by_restore["entry"] == expected + assert scene._variable_records.variables == {VARIABLE_ID: expected} + + +def test_load_fills_a_null_magnitude_range_with_the_default(scene, registry): + """#15: a null magnitudeRange in the file loads at the default, logged at DEBUG.""" + _seed_part_variables(registry, [_pressure_variable()]) + + with patch(AGGREGATE_LOGGER) as mock_logger: + _apply(scene, _runtime_state({}), _file_entries(magnitude_range=None, ranges=((3.0, 4.0),))) + + assert scene._variable_records.variables == { + VARIABLE_ID: _pressure_record(magnitude_range=(0.0, 49.0), ranges=[(3.0, 4.0)]) + } + assert mock_logger.debug.call_count == 1 + assert mock_logger.warning.call_count == 0 + + +def test_load_drops_a_file_id_with_no_record(scene, registry): + """#16: a file id no dataset carries is dropped with a WARNING.""" + _seed_part_variables(registry, [_pressure_variable()]) + ghost = VisorVariableState( + id="POINT::ghost::1", array_name="ghost", type=VisorVtkVariableType.POINT, + num_components=1, magnitude_range=(5.0, 6.0), ranges=[(5.0, 6.0)], + ) + + with patch(AGGREGATE_LOGGER) as mock_logger: + _apply(scene, _runtime_state({}), {"POINT::ghost::1": ghost}) + + assert scene._variable_records.variables == {VARIABLE_ID: _pressure_record()} + assert mock_logger.warning.call_count == 1 + + +def test_set_state_push_carries_the_record(scene, registry): + """#18: the state handed to the push carries the record, not the file entry, as a copy.""" + _seed_part_variables(registry, [_pressure_variable()]) + pushed = {} + scene._push_runtime_state = lambda state: pushed.update(state=state) + + _apply(scene, _runtime_state({}), _file_entries(magnitude_range=(1.0, 2.0), ranges=((3.0, 4.0),))) + + entry = pushed["state"].scene.variable_states[VARIABLE_ID] + assert entry == _pressure_record(magnitude_range=(1.0, 2.0), ranges=[(3.0, 4.0)]) + assert entry is not scene._variable_records.variables[VARIABLE_ID] + assert pushed["state"].model_dump(by_alias=True)["scene"]["variableStates"][VARIABLE_ID] == { + "id": VARIABLE_ID, + "arrayName": "pressure", + "type": "POINT", + "numComponents": 1, + "partIds": [NODE_ID], + "defaultMagnitudeRange": (0.0, 49.0), + "defaultRanges": [(10.0, 20.0)], + "magnitudeRange": (1.0, 2.0), + "ranges": [(3.0, 4.0)], + } + + +def _contradictory_reply(): + """The browser's reply: a different range for the same id, a stray id, and another unit.""" + return RuntimeAppState.from_components( + ui=VisorUIState(dark_theme=False), + unit="ft", + dataset_states={}, + variable_states={ + VARIABLE_ID: _pressure_record(magnitude_range=(900.0, 901.0), ranges=[(902.0, 903.0)]), + "POINT::stray::1": _pressure_record(id="POINT::stray::1", array_name="stray"), + }, + ) + + +def test_get_state_takes_the_variable_states_from_the_server(scene, registry): + """#19: the save receives the server's record, whatever the browser replied.""" + _seed_part_variables(registry, [_pressure_variable()]) + scene._rebuild_variable_records("add") + captured = _capture_persist_input(scene, _contradictory_reply()) + + asyncio.run(scene.get_state(timeout=1.0)) + + assert captured["runtime_state"].scene.variable_states == {VARIABLE_ID: _pressure_record()} + + +def test_get_state_takes_the_unit_from_the_registry(scene, registry): + """#20: the save receives the registry's unit, whatever the browser replied.""" + registry.unit = "mm" + captured = _capture_persist_input(scene, _contradictory_reply()) + + asyncio.run(scene.get_state(timeout=1.0)) + + assert captured["runtime_state"].scene.unit == "mm" diff --git a/tests/unit/vtk/scene/test_visor_state_mapper.py b/tests/unit/vtk/scene/test_visor_state_mapper.py index 80f43608..eeeb1d45 100644 --- a/tests/unit/vtk/scene/test_visor_state_mapper.py +++ b/tests/unit/vtk/scene/test_visor_state_mapper.py @@ -1,7 +1,27 @@ import pytest +from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType +from ansys.visor.viewer.models.common.visor_ui_state import VisorUIState +from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecord +from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState +from ansys.visor.viewer.models.runtime.scene.runtime_app_state import RuntimeAppState from ansys.visor.viewer.vtk.scene.visor_state_mapper import VisorStateMapper + +def _literal_record() -> VisorVariableRecord: + """A record whose custom range (2.0, 3.0) differs from its default (0.0, 10.0).""" + return VisorVariableRecord( + id="POINT::pressure::1", + array_name="pressure", + type=VisorVtkVariableType.POINT, + num_components=1, + part_ids=[1], + default_magnitude_range=(0.0, 10.0), + default_ranges=[(0.0, 10.0)], + magnitude_range=(2.0, 3.0), + ranges=[(2.0, 3.0)], + ) + # ------------------------------------------------------------------ # Helpers / fakes # ------------------------------------------------------------------ @@ -66,7 +86,7 @@ def fake_from_components(**kwargs): "scene": type("Scene", (), { "unit": "m", "camera": "cam", - "variable_states": {"var": 1}, + "variable_states": {"POINT::pressure::1": _literal_record()}, "dataset_states": {1: {"state": 123}}, "cross_section": "cs", "orthographic_enabled": True, @@ -83,7 +103,40 @@ def fake_from_components(**kwargs): assert called["datasets"] == {"dsA": {"converted": {"state": 123}}} assert called["unit"] == "m" assert called["camera"] == "cam" - assert called["variable_states"] == {"var": 1} + # Rewritten in place (3.5.1 increment 1): the record is projected, not passed through. + assert called["variable_states"] == { + "POINT::pressure::1": VisorVariableState( + id="POINT::pressure::1", + array_name="pressure", + type=VisorVtkVariableType.POINT, + num_components=1, + magnitude_range=(2.0, 3.0), + ranges=[(2.0, 3.0)], + ) + } + + +def test_runtime_to_persisted_projects_records_onto_the_persisted_entry(): + """#22: through the real persisted model, the saved entry carries the effective range and no default keys.""" + registry = FakeRegistry(datasets={}) + runtime_app_state = RuntimeAppState.from_components( + ui=VisorUIState(), + unit="m", + dataset_states={}, + variable_states={"POINT::pressure::1": _literal_record()}, + ) + + persisted = VisorStateMapper(registry).runtime_to_persisted(runtime_app_state) + + entry = persisted.model_dump(by_alias=True)["scene"]["variable_states"]["POINT::pressure::1"] + assert entry == { + "id": "POINT::pressure::1", + "arrayName": "pressure", + "type": "POINT", + "numComponents": 1, + "magnitudeRange": (2.0, 3.0), + "ranges": [(2.0, 3.0)], + } def test_runtime_to_persisted_skips_missing_dataset(monkeypatch): diff --git a/tests/unit/vtk/test_wire_format_identity.py b/tests/unit/vtk/test_wire_format_identity.py index 81ea3757..886ccc42 100644 --- a/tests/unit/vtk/test_wire_format_identity.py +++ b/tests/unit/vtk/test_wire_format_identity.py @@ -350,3 +350,99 @@ def test_data_array_type_is_str_and_point_or_cell_in_emitted_json(): assert d["type"] in {"POINT", "CELL"} +# --------------------------------------------------------------------------- +# 3.5.1 increment 1: the server's variable records on the scene-details wire. +# +# The registry holds two hand-written datasets sharing "pressure"; the scene +# graph is the fixture sphere and plays no part in the variables. Expected +# values are hand-written literals. +# --------------------------------------------------------------------------- + +class _VariablesOnlyDataset: + """Dataset stand-in answering the two reads get_scene_details and the rebuild make.""" + + def __init__(self, dataset_id, parts): + from ansys.visor.viewer.models.runtime.dataset.runtime_dataset_state import ( + RuntimeDatasetState, + ) + self.state = RuntimeDatasetState(id=dataset_id, part_states={}) + self._parts = parts + + def list_variables(self): + from ansys.visor.viewer.vtk.variables.visor_part_variables import VisorPartVariables + return [ + VisorPartVariables(part_id=part_id, part_name=f"p{part_id}", variables=variables) + for part_id, variables in self._parts.items() + ] + + +def _variable(name, ranges, magnitude): + from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType + from ansys.visor.viewer.vtk.variables.visor_variables import VisorVariable + return VisorVariable( + index=0, type=VisorVtkVariableType.POINT, name=name, num_components=len(ranges), + num_points=4, ranges=list(ranges), magnitude_range=magnitude, + ) + + +def _scene_with_two_datasets_sharing_pressure(): + scene = _build_scene(_sphere_polydata()) + scene._dataset_registry.datasets = { + 1: _VariablesOnlyDataset(1, { + 11: [ + _variable("pressure", [(0.0, 10.0)], (0.0, 10.0)), + _variable("velocity", [(0.0, 1.0), (0.0, 2.0), (0.0, 3.0)], (0.0, 4.0)), + ], + }), + 2: _VariablesOnlyDataset(2, {21: [_variable("pressure", [(-5.0, 4.0)], (-5.0, 4.0))]}), + } + scene._rebuild_variable_records("add") + return scene + + +def test_scene_details_json_carries_every_record_key_and_no_null(): + """#17: every record key for every variable, no null, on two datasets sharing one variable.""" + scene = _scene_with_two_datasets_sharing_pressure() + + payload = json.loads(scene.get_scene_details_json()) + + assert payload["appState"]["scene"]["variableStates"] == { + "POINT::pressure::1": { + "id": "POINT::pressure::1", + "arrayName": "pressure", + "type": "POINT", + "numComponents": 1, + "partIds": [11, 21], + "defaultMagnitudeRange": [-5.0, 10.0], + "defaultRanges": [[-5.0, 10.0]], + "magnitudeRange": [-5.0, 10.0], + "ranges": [[-5.0, 10.0]], + }, + "POINT::velocity::3": { + "id": "POINT::velocity::3", + "arrayName": "velocity", + "type": "POINT", + "numComponents": 3, + "partIds": [11], + "defaultMagnitudeRange": [0.0, 4.0], + "defaultRanges": [[0.0, 1.0], [0.0, 2.0], [0.0, 3.0]], + "magnitudeRange": [0.0, 4.0], + "ranges": [[0.0, 1.0], [0.0, 2.0], [0.0, 3.0]], + }, + } + + +def test_mutating_the_handed_out_records_leaves_the_holder_unchanged(): + """#24: get_scene_details hands out a deep copy; mutating its entries does not reach the holder.""" + scene = _scene_with_two_datasets_sharing_pressure() + + details = scene.get_scene_details() + handed_out = details.app_state.scene.variable_states["POINT::pressure::1"] + handed_out.magnitude_range = (900.0, 901.0) + handed_out.ranges[0] = (902.0, 903.0) + handed_out.part_ids.append(999) + + held = scene._variable_records.variables["POINT::pressure::1"] + assert held.magnitude_range == (-5.0, 10.0) + assert held.ranges == [(-5.0, 10.0)] + assert held.part_ids == [11, 21] diff --git a/tests/unit/vtk/variables/test_visor_variable_aggregate.py b/tests/unit/vtk/variables/test_visor_variable_aggregate.py new file mode 100644 index 00000000..9d2b214f --- /dev/null +++ b/tests/unit/vtk/variables/test_visor_variable_aggregate.py @@ -0,0 +1,106 @@ +"""Tests for the pure variable aggregate (3.5.1 increment 1, tests 5-9). + +The registry is a hand-written fake; every expected value is a hand-written +literal, never recomputed the way the code computes it. +""" +from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType +from ansys.visor.viewer.vtk.variables.visor_part_variables import VisorPartVariables +from ansys.visor.viewer.vtk.variables.visor_variable_aggregate import build_variable_records +from ansys.visor.viewer.vtk.variables.visor_variables import VisorVariable + +POINT = VisorVtkVariableType.POINT + + +def _var(name, ranges, magnitude, point_type=POINT): + return VisorVariable( + index=0, type=point_type, name=name, num_components=len(ranges), + num_points=4, ranges=list(ranges), magnitude_range=magnitude, + ) + + +class _FakeDataset: + def __init__(self, parts): + self._parts = parts + + def list_variables(self): + return [ + VisorPartVariables(part_id=part_id, part_name=f"p{part_id}", variables=variables) + for part_id, variables in self._parts.items() + ] + + +class _FakeRegistry: + def __init__(self, *datasets): + self.datasets = {i + 1: d for i, d in enumerate(datasets)} + + +def _two_datasets_sharing_pressure(): + return _FakeRegistry( + _FakeDataset({1: [_var("pressure", [(0.0, 10.0)], (0.0, 10.0))]}), + _FakeDataset({3: [_var("pressure", [(-5.0, 4.0)], (-5.0, 4.0))]}), + ) + + +def test_participation_is_the_union_of_parts_across_datasets(): + """#5: one record for the shared variable, naming both parts.""" + records = build_variable_records(_two_datasets_sharing_pressure(), {}) + + assert list(records) == ["POINT::pressure::1"] + assert records["POINT::pressure::1"].part_ids == [1, 3] + + +def test_default_ranges_widen_to_the_min_of_mins_and_max_of_maxes(): + """#6: (0.0, 10.0) and (-5.0, 4.0) widen to (-5.0, 10.0); a new id's custom equals its default.""" + record = build_variable_records(_two_datasets_sharing_pressure(), {})["POINT::pressure::1"] + + assert record.default_magnitude_range == (-5.0, 10.0) + assert record.default_ranges == [(-5.0, 10.0)] + assert record.magnitude_range == (-5.0, 10.0) + assert record.ranges == [(-5.0, 10.0)] + + +def test_a_width_split_produces_distinct_ids(): + """#7: same name and association at widths 3 and 1 are two records, one part each.""" + registry = _FakeRegistry( + _FakeDataset({ + 1: [_var("velocity", [(0.0, 1.0), (0.0, 2.0), (0.0, 3.0)], (0.0, 4.0))], + 2: [_var("velocity", [(5.0, 6.0)], (5.0, 6.0))], + }) + ) + + records = build_variable_records(registry, {}) + + assert sorted(records) == ["POINT::velocity::1", "POINT::velocity::3"] + assert records["POINT::velocity::3"].part_ids == [1] + assert records["POINT::velocity::3"].default_ranges == [(0.0, 1.0), (0.0, 2.0), (0.0, 3.0)] + assert records["POINT::velocity::1"].part_ids == [2] + + +def _one_dataset_pressure(): + return _FakeRegistry(_FakeDataset({1: [_var("pressure", [(0.0, 10.0)], (0.0, 10.0))]})) + + +def test_an_edited_custom_range_is_kept_across_widening(): + """#8: a custom slot that differs from its previous default survives the rebuild.""" + previous = build_variable_records(_one_dataset_pressure(), {}) + previous["POINT::pressure::1"] = previous["POINT::pressure::1"].model_copy( + update={"magnitude_range": (2.0, 3.0), "ranges": [(2.0, 3.0)]} + ) + + record = build_variable_records(_two_datasets_sharing_pressure(), previous)["POINT::pressure::1"] + + assert record.default_magnitude_range == (-5.0, 10.0) + assert record.magnitude_range == (2.0, 3.0) + assert record.ranges == [(2.0, 3.0)] + + +def test_an_unedited_custom_range_follows_the_new_default(): + """#9: a custom slot equal to its previous default (0.0, 10.0) follows the widening to (-5.0, 10.0).""" + previous = build_variable_records(_one_dataset_pressure(), {}) + assert previous["POINT::pressure::1"].magnitude_range == (0.0, 10.0) + + record = build_variable_records(_two_datasets_sharing_pressure(), previous)["POINT::pressure::1"] + + assert record.magnitude_range == (-5.0, 10.0) + assert record.ranges == [(-5.0, 10.0)] + diff --git a/tests/unit/vtk/variables/test_visor_variable_empty_block.py b/tests/unit/vtk/variables/test_visor_variable_empty_block.py new file mode 100644 index 00000000..4db23d6c --- /dev/null +++ b/tests/unit/vtk/variables/test_visor_variable_empty_block.py @@ -0,0 +1,72 @@ +"""G4: variable metadata on a multiblock with an empty (None) block (3.5.1 increment 1, test 23). + +The scene graph and the dataset's PartIndex must name the same parts with the +same variables, or the record's ``part_ids`` name ids the client's nodes lack. + +Expected to fail today on F-E: ``VisorGroupNode._post_init`` walks every +``GetBlock(i)`` including ``None``, and node creation raises RuntimeError. +PartIndex skips empty blocks. Recorded as found-not-fixed; neither +part_index.py nor group_node.py is edited in this increment. +""" +import pytest +from vtkmodules.vtkCommonCore import vtkFloatArray +from vtkmodules.vtkCommonDataModel import vtkMultiBlockDataSet +from vtkmodules.vtkFiltersSources import vtkSphereSource + +from ansys.visor.viewer.core.metadata import ExtendedMetadata +from ansys.visor.viewer.vtk.datasets.visor_dataset import VisorDataset +from ansys.visor.viewer.vtk.scene_graph import VisorSceneGraph + + +def _sphere_with_point_array(name: str): + src = vtkSphereSource() + src.Update() + poly = src.GetOutput() + array = vtkFloatArray() + array.SetName(name) + array.SetNumberOfComponents(1) + array.SetNumberOfTuples(poly.GetNumberOfPoints()) + for i in range(poly.GetNumberOfPoints()): + array.SetTuple1(i, float(i)) + poly.GetPointData().AddArray(array) + return poly + + +def _multiblock_with_an_empty_block() -> vtkMultiBlockDataSet: + mb = vtkMultiBlockDataSet() + mb.SetNumberOfBlocks(3) + mb.SetBlock(0, _sphere_with_point_array("pressure")) + # Block 1 is left None: the empty block. + mb.SetBlock(2, _sphere_with_point_array("temperature")) + return mb + + +@pytest.mark.xfail( + strict=True, + raises=RuntimeError, + reason="F-E: VisorGroupNode._post_init walks the None block and node creation raises " + "RuntimeError; PartIndex skips it. Found, not fixed, in 3.5.1 increment 1.", +) +def test_part_node_data_arrays_and_list_variables_agree_on_an_empty_block(): + """#23: per part id, part_node.data_arrays and dataset.list_variables() name the same arrays.""" + mb = _multiblock_with_an_empty_block() + + graph = VisorSceneGraph() + graph.load_dataset(mb, "ds") + part_nodes = graph.get_descendant_part_nodes(include_self=False) + node_ids = [node.id for node in part_nodes] + dataset = VisorDataset(1, "ds", mb, node_ids, ExtendedMetadata(name="ds", unit="m")) + + # Hand-written, in leaf order: the two non-empty blocks. + expected_names = [["pressure"], ["temperature"]] + assert len(node_ids) == 2 + expected = dict(zip(node_ids, expected_names)) + + from_graph = {node.id: [info.name for info in node.data_arrays] for node in part_nodes} + from_dataset = { + part.part_id: [variable.name for variable in part.variables] + for part in dataset.list_variables() + } + assert from_graph == expected + assert from_dataset == expected + From a5780fa1d5f5d9ccd5f56fcb231d9d9b564be85c Mon Sep 17 00:00:00 2001 From: Laura Kasian Date: Tue, 29 Sep 2026 09:53:59 -0700 Subject: [PATCH 2/7] feat: clean up server side variable aggregation --- .../models/common/visor_variable_record.py | 166 +++++++++++++++++- .../requests/visor_save_state_response.py | 2 + src/ansys/visor/viewer/vtk/scene/base.py | 56 +++--- .../vtk/variables/visor_variable_aggregate.py | 153 ---------------- .../test_visor_variable_record_build.py} | 19 +- tests/unit/vtk/scene/test_base.py | 30 ++-- tests/unit/vtk/test_wire_format_identity.py | 2 +- 7 files changed, 222 insertions(+), 206 deletions(-) delete mode 100644 src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py rename tests/unit/{vtk/variables/test_visor_variable_aggregate.py => models/test_visor_variable_record_build.py} (78%) diff --git a/src/ansys/visor/viewer/models/common/visor_variable_record.py b/src/ansys/visor/viewer/models/common/visor_variable_record.py index ca188fdc..877a1dc4 100644 --- a/src/ansys/visor/viewer/models/common/visor_variable_record.py +++ b/src/ansys/visor/viewer/models/common/visor_variable_record.py @@ -1,13 +1,23 @@ """Server-owned record for one color variable across every dataset in the scene.""" -from typing import Dict, List, Tuple +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, Dict, List, Tuple from pydantic import BaseModel, ConfigDict, Field, field_serializer from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType +from ansys.visor.viewer.core.visor_logging import VisorDefaultLogger from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState from ansys.visor.viewer.models.persist.scene.persisted_scene_state import _IDENTIFIER_SEPARATOR +if TYPE_CHECKING: + from ansys.visor.viewer.vtk.datasets.visor_dataset_registry import VisorDatasetRegistry + from ansys.visor.viewer.vtk.variables.visor_variables import VisorVariable + +logger = VisorDefaultLogger(__name__) + +Range = Tuple[float, float] + def compose_variable_identifier(type: VisorVtkVariableType, name: str, num_components: int) -> str: """Compose the stable variable identifier ``::::``. @@ -58,6 +68,94 @@ def to_variable_state(self) -> VisorVariableState: ranges=list(self.ranges), ) + def range_for(self, component: int) -> Range | None: + """Return the effective range for a slot: ``-1`` is magnitude, ``0..n-1`` a component, else None.""" + if component == -1: + return self.magnitude_range + if 0 <= component < self.num_components and component < len(self.ranges): + return self.ranges[component] + return None + + +@dataclass +class _VariableAccumulator: + """Participation and widened default ranges for one (association, name, width) key.""" + + variable_type: VisorVtkVariableType + name: str + num_components: int + part_ids: List[int] = field(default_factory=list, init=False) + magnitude: Range | None = field(default=None, init=False) + components: List[Range | None] = field(default_factory=list, init=False) + + def __post_init__(self) -> None: + """Start with one empty default slot per component.""" + self.components = [None] * self.num_components + + @staticmethod + def _widen(current: Range | None, other: Range | None) -> Range | None: + """Return the union of two ranges; ``None`` contributes nothing.""" + if other is None: + return current + other = (float(other[0]), float(other[1])) + if current is None: + return other + return (min(current[0], other[0]), max(current[1], other[1])) + + @staticmethod + def _follow_or_keep(previous_custom: Range, previous_default: Range, new_default: Range) -> Range: + """A custom slot equal to its previous default follows the new default; otherwise it is kept.""" + if tuple(previous_custom) == tuple(previous_default): + return new_default + return tuple(previous_custom) + + @property + def variable_id(self) -> str: + """The composed identifier for this key: the one compose_variable_identifier call site.""" + return compose_variable_identifier(self.variable_type, self.name, self.num_components) + + def add_part(self, part_id: int, variable: "VisorVariable") -> None: + """Record the part's participation and widen the default ranges with its variable's ranges.""" + if part_id not in self.part_ids: + self.part_ids.append(part_id) + + variable_magnitude = variable.magnitude_range + if variable_magnitude is None and self.num_components == 1 and variable.ranges: + variable_magnitude = variable.ranges[0] + self.magnitude = self._widen(self.magnitude, variable_magnitude) + + for k in range(min(self.num_components, len(variable.ranges))): + self.components[k] = self._widen(self.components[k], variable.ranges[k]) + + def to_record(self, previous_record: VisorVariableRecord | None) -> VisorVariableRecord: + """Build the record, carrying each custom slot from ``previous_record`` by the D3 rule.""" + n = self.num_components + default_magnitude = self.magnitude or (0.0, 0.0) + default_ranges = [slot if slot is not None else (0.0, 0.0) for slot in self.components] + + custom_magnitude = default_magnitude + custom_ranges = list(default_ranges) + old = previous_record + if old is not None: + custom_magnitude = self._follow_or_keep(old.magnitude_range, old.default_magnitude_range, default_magnitude) + custom_ranges = [ + self._follow_or_keep(old.ranges[k], old.default_ranges[k], default_ranges[k]) + if k < len(old.ranges) and k < len(old.default_ranges) else default_ranges[k] + for k in range(n) + ] + + return VisorVariableRecord( + id=self.variable_id, + array_name=self.name, + type=self.variable_type, + num_components=n, + part_ids=sorted(self.part_ids), + default_magnitude_range=default_magnitude, + default_ranges=default_ranges, + magnitude_range=custom_magnitude, + ranges=custom_ranges, + ) + class VisorVariableRecords(BaseModel): """Holder for the scene's variable records, keyed by identifier. @@ -67,3 +165,69 @@ class VisorVariableRecords(BaseModel): """ variables: Dict[str, VisorVariableRecord] = Field(default_factory=dict) + @classmethod + def from_registry( + cls, + registry: "VisorDatasetRegistry", + previous: "VisorVariableRecords", + ) -> "VisorVariableRecords": + """Build a new holder from the registry's per-part variables, carrying custom ranges from ``previous``. + + 1. Union: each part carrying a variable joins that identifier's ``part_ids``. + 2. Widen: default ranges are the min of the mins and the max of the maxes. + 3. Custom: an existing id keeps an edited custom slot; an unedited slot (equal to the + previous default) follows the new default. A new id starts with custom equal to default. + + ``previous`` is never mutated. + """ + accumulators: Dict[tuple, _VariableAccumulator] = {} + for dataset in list(registry.datasets.values()): + for part_variables in dataset.list_variables(): + for variable in part_variables.variables: + key = (variable.type, variable.name, int(variable.num_components)) + if key not in accumulators: + accumulators[key] = _VariableAccumulator(*key) + accumulators[key].add_part(part_variables.part_id, variable) + + variables: Dict[str, VisorVariableRecord] = {} + for accumulator in accumulators.values(): + variable_id = accumulator.variable_id + variables[variable_id] = accumulator.to_record(previous.variables.get(variable_id)) + return cls(variables=variables) + + def overlay(self, file_states: Dict[str, VisorVariableState]) -> "VisorVariableRecords": + """Apply a file's ranges to this fresh build by the D3 load rule and return this holder. + + - A null or absent ``magnitudeRange`` falls back to the default (DEBUG). + - ``ranges`` whose length differs from ``num_components`` fall back to the default (DEBUG). + - A file id with no record is dropped (WARNING). + + Assigns a new dict to ``variables``; the previous dict and its entries are never mutated. + """ + result = dict(self.variables) + for variable_id, entry in (file_states or {}).items(): + record = result.get(variable_id) + if record is None: + logger.warning("persisted variable %s has no record in the scene; dropped", variable_id) + continue + + magnitude = entry.magnitude_range + if magnitude is None: + logger.debug("persisted variable %s has no magnitudeRange; default used", variable_id) + magnitude = record.default_magnitude_range + + ranges = entry.ranges + if ranges is None or len(ranges) != record.num_components: + logger.debug( + "persisted variable %s has %s ranges for %d components; default used", + variable_id, None if ranges is None else len(ranges), record.num_components, + ) + ranges = record.default_ranges + + result[variable_id] = record.model_copy( + update={"magnitude_range": tuple(magnitude), "ranges": [tuple(r) for r in ranges]}, + deep=True, + ) + self.variables = result + return self + diff --git a/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py b/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py index 79f05614..9b45ef5b 100644 --- a/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py +++ b/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py @@ -24,6 +24,8 @@ class VisorSaveStateResponse(BaseModel): request_id: int = Field(alias="requestId") app_state: RuntimeAppState = Field(alias="appState") + # Guards a client that still sends variableStates in the pre-record shape. + # A later increment decides whether this validator stays. @model_validator(mode="before") @classmethod def _discard_browser_variable_states(cls, data: Any) -> Any: diff --git a/src/ansys/visor/viewer/vtk/scene/base.py b/src/ansys/visor/viewer/vtk/scene/base.py index 0853d347..a8b9abb2 100644 --- a/src/ansys/visor/viewer/vtk/scene/base.py +++ b/src/ansys/visor/viewer/vtk/scene/base.py @@ -26,10 +26,6 @@ from ansys.visor.viewer.vtk.scene.visor_state_mapper import VisorStateMapper from ansys.visor.viewer.vtk.scene_graph import VisorSceneGraph from ansys.visor.viewer.vtk.variables.visor_part_variables import VisorPartVariables -from ansys.visor.viewer.vtk.variables.visor_variable_aggregate import ( - build_variable_records, - overlay_persisted_ranges, -) from ansys.visor.viewer.vtk.variables.visor_variable_update import VisorVariableUpdate if TYPE_CHECKING: @@ -110,8 +106,9 @@ def __init__( ) # The server's variable records, keyed by composed identifier. Created once - # here and never rebound. Written only by _rebuild_variable_records, under - # _vtk_lock, by assigning a new dict (copy-on-write) so the unlocked reader in + # here and never rebound. Written only by _rebuild_variable_records_from_registry + # and _load_variable_records, under _vtk_lock, by assigning a new dict + # (copy-on-write) so the unlocked reader in # get_scene_details never sees a dict change size. Handed out only as # model_copy(deep=True): a shallow copy would share the dict and its entries. self._variable_records = VisorVariableRecords() @@ -303,7 +300,7 @@ def apply_state(self, state: PersistedViewerStateV1): # Variable records: rebuilt from the registry and overlaid with the file's # ranges, then placed on the runtime state (CC-1), all before # _restore_part_states, which reads each colored part's range from that dict. - self._rebuild_variable_records("load", persisted=state.scene.variable_states) + self._load_variable_records(state.scene.variable_states) runtime_app_state.scene.variable_states = self._variable_records.model_copy(deep=True).variables # One call per state class: updates the server's stored state and its VTK objects. @@ -402,7 +399,7 @@ def clear(self): self._renderer.deregister_all() self._dataset_registry.clear() self._scene_graph = None - self._rebuild_variable_records("clear") + self._rebuild_variable_records_from_registry() def populate_scene(self): """Update widgets to reflect the current scene contents. @@ -488,7 +485,7 @@ def add_dataset(self, input: VisorDatasetType, metadata: ExtendedMetadata) -> in node_ids = [leaf.id for leaf in part_nodes] self._dataset_registry.add(dataset_id, dataset_name, input, node_ids, metadata) - self._rebuild_variable_records("add") + self._rebuild_variable_records_from_registry() return dataset_id @@ -520,7 +517,7 @@ def remove_dataset(self, dataset_id: int): # Unregister the dataset self._dataset_registry.remove(dataset_id) - self._rebuild_variable_records("remove") + self._rebuild_variable_records_from_registry() def list_variables_for_dataset(self, dataset_id: int) -> List[VisorPartVariables]: return self._dataset_registry.list_variables(dataset_id) @@ -546,35 +543,38 @@ def update_variables_for_dataset(self, dataset_id: int, variables: List[VisorVar dataset_node.refresh_descendant_variable_metadata(include_self=True) # After the reload and the metadata refresh: a width change changes the id. - self._rebuild_variable_records("update_variables") + self._rebuild_variable_records_from_registry() with timer.phase("render"): self.render() timer.log() - def _rebuild_variable_records( - self, - point: str, - persisted: Dict[str, VisorVariableState] | None = None, - ) -> None: - """Rebuild the variable records from the registry. + def _rebuild_variable_records_from_registry(self) -> None: + """Rebuild the held records from the registry, carrying custom ranges; under ``_vtk_lock``. + + Used at add, remove, clear and update. Assigns a new dict to the holder + (copy-on-write); never mutates the live dict or its entries. + """ + with self._vtk_lock: + records = VisorVariableRecords.from_registry(self._dataset_registry, self._variable_records).variables + self._variable_records.variables = records + logger.debug("variable records rebuilt from registry: %d records", len(records)) - ``persisted`` is ``None`` at add, remove, clear and update: the previous - records are carried by the custom-range rule. On load it is the file's - ``variable_states``: the records are built fresh and the file's ranges - overlaid. + def _load_variable_records(self, file_states: Dict[str, VisorVariableState]) -> None: + """Build the held records fresh from the registry and overlay the file's ranges; under ``_vtk_lock``. - Under ``_vtk_lock``. Assigns a new dict to the holder (copy-on-write); - never mutates the live dict or its entries. + Used at load. Assigns a new dict to the holder (copy-on-write); never + mutates the live dict or its entries. """ with self._vtk_lock: - if persisted is None: - records = build_variable_records(self._dataset_registry, self._variable_records.variables) - else: - records = overlay_persisted_ranges(build_variable_records(self._dataset_registry, {}), persisted) + records = ( + VisorVariableRecords.from_registry(self._dataset_registry, VisorVariableRecords()) + .overlay(file_states) + .variables + ) self._variable_records.variables = records - logger.debug("variable records rebuilt at %s: %d records", point, len(records)) + logger.debug("variable records loaded from file: %d records", len(records)) def render(self): """Delegate to the renderer backend. diff --git a/src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py b/src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py deleted file mode 100644 index 397e49e6..00000000 --- a/src/ansys/visor/viewer/vtk/variables/visor_variable_aggregate.py +++ /dev/null @@ -1,153 +0,0 @@ -"""Pure aggregation of the scene's variable records from the dataset registry.""" - -from typing import TYPE_CHECKING, Dict, List, Tuple - -from ansys.visor.viewer.core.visor_logging import VisorDefaultLogger -from ansys.visor.viewer.models.common.visor_variable_record import ( - VisorVariableRecord, - compose_variable_identifier, -) -from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState - -if TYPE_CHECKING: - from ansys.visor.viewer.vtk.datasets.visor_dataset_registry import VisorDatasetRegistry - -logger = VisorDefaultLogger(__name__) - -Range = Tuple[float, float] - - -def _widen(current: Range | None, other: Range | None) -> Range | None: - """Return the union of two ranges; ``None`` contributes nothing.""" - if other is None: - return current - other = (float(other[0]), float(other[1])) - if current is None: - return other - return (min(current[0], other[0]), max(current[1], other[1])) - - -def _follow_or_keep(previous_custom: Range, previous_default: Range, new_default: Range) -> Range: - """A custom slot equal to its previous default follows the new default; otherwise it is kept.""" - if tuple(previous_custom) == tuple(previous_default): - return new_default - return tuple(previous_custom) - - -def build_variable_records( - registry: "VisorDatasetRegistry", - previous: Dict[str, VisorVariableRecord], -) -> Dict[str, VisorVariableRecord]: - """Build the records from every dataset's variables, carrying custom ranges from ``previous``. - - 1. Union: each part carrying a variable joins that identifier's ``part_ids``. - 2. Widen: default ranges are the min of the mins and the max of the maxes. - 3. Custom: an existing id keeps an edited custom slot; an unedited slot (equal to the - previous default) follows the new default. A new id starts with custom equal to default. - - Returns a new dict of new records; ``previous`` is never mutated. - """ - parts: Dict[str, List[int]] = {} - identity: Dict[str, tuple] = {} - magnitude: Dict[str, Range | None] = {} - components: Dict[str, List[Range | None]] = {} - - for dataset in list(registry.datasets.values()): - for part_variables in dataset.list_variables(): - for variable in part_variables.variables: - n = int(variable.num_components) - variable_id = compose_variable_identifier(variable.type, variable.name, n) - if variable_id not in identity: - identity[variable_id] = (variable.name, variable.type, n) - parts[variable_id] = [] - magnitude[variable_id] = None - components[variable_id] = [None] * n - if part_variables.part_id not in parts[variable_id]: - parts[variable_id].append(part_variables.part_id) - - variable_magnitude = variable.magnitude_range - if variable_magnitude is None and n == 1 and variable.ranges: - variable_magnitude = variable.ranges[0] - magnitude[variable_id] = _widen(magnitude[variable_id], variable_magnitude) - - slots = components[variable_id] - for k in range(min(n, len(variable.ranges))): - slots[k] = _widen(slots[k], variable.ranges[k]) - - records: Dict[str, VisorVariableRecord] = {} - for variable_id, (name, variable_type, n) in identity.items(): - default_magnitude = magnitude[variable_id] or (0.0, 0.0) - default_ranges = [slot if slot is not None else (0.0, 0.0) for slot in components[variable_id]] - - custom_magnitude = default_magnitude - custom_ranges = list(default_ranges) - old = previous.get(variable_id) - if old is not None: - custom_magnitude = _follow_or_keep(old.magnitude_range, old.default_magnitude_range, default_magnitude) - custom_ranges = [ - _follow_or_keep(old.ranges[k], old.default_ranges[k], default_ranges[k]) - if k < len(old.ranges) and k < len(old.default_ranges) else default_ranges[k] - for k in range(n) - ] - - records[variable_id] = VisorVariableRecord( - id=variable_id, - array_name=name, - type=variable_type, - num_components=n, - part_ids=sorted(parts[variable_id]), - default_magnitude_range=default_magnitude, - default_ranges=default_ranges, - magnitude_range=custom_magnitude, - ranges=custom_ranges, - ) - return records - - -def overlay_persisted_ranges( - records: Dict[str, VisorVariableRecord], - persisted: Dict[str, VisorVariableState], -) -> Dict[str, VisorVariableRecord]: - """Overlay the file's custom ranges onto freshly built records. - - - A null or absent ``magnitudeRange`` falls back to the default (DEBUG). - - ``ranges`` whose length differs from ``num_components`` fall back to the default (DEBUG). - - A file id with no record is dropped (WARNING). - - Returns a new dict; ``records`` is never mutated. - """ - result = dict(records) - for variable_id, entry in (persisted or {}).items(): - record = result.get(variable_id) - if record is None: - logger.warning("persisted variable %s has no record in the scene; dropped", variable_id) - continue - - magnitude = entry.magnitude_range - if magnitude is None: - logger.debug("persisted variable %s has no magnitudeRange; default used", variable_id) - magnitude = record.default_magnitude_range - - ranges = entry.ranges - if ranges is None or len(ranges) != record.num_components: - logger.debug( - "persisted variable %s has %s ranges for %d components; default used", - variable_id, None if ranges is None else len(ranges), record.num_components, - ) - ranges = record.default_ranges - - result[variable_id] = record.model_copy( - update={"magnitude_range": tuple(magnitude), "ranges": [tuple(r) for r in ranges]}, - deep=True, - ) - return result - - -def resolve_record_range(record: VisorVariableRecord, component: int) -> Range | None: - """Return the effective range for a component slot: ``-1`` is magnitude, ``0..n-1`` a component.""" - if component == -1: - return record.magnitude_range - if 0 <= component < record.num_components and component < len(record.ranges): - return record.ranges[component] - return None - diff --git a/tests/unit/vtk/variables/test_visor_variable_aggregate.py b/tests/unit/models/test_visor_variable_record_build.py similarity index 78% rename from tests/unit/vtk/variables/test_visor_variable_aggregate.py rename to tests/unit/models/test_visor_variable_record_build.py index 9d2b214f..81142253 100644 --- a/tests/unit/vtk/variables/test_visor_variable_aggregate.py +++ b/tests/unit/models/test_visor_variable_record_build.py @@ -5,7 +5,7 @@ """ from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType from ansys.visor.viewer.vtk.variables.visor_part_variables import VisorPartVariables -from ansys.visor.viewer.vtk.variables.visor_variable_aggregate import build_variable_records +from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecords from ansys.visor.viewer.vtk.variables.visor_variables import VisorVariable POINT = VisorVtkVariableType.POINT @@ -43,7 +43,7 @@ def _two_datasets_sharing_pressure(): def test_participation_is_the_union_of_parts_across_datasets(): """#5: one record for the shared variable, naming both parts.""" - records = build_variable_records(_two_datasets_sharing_pressure(), {}) + records = VisorVariableRecords.from_registry(_two_datasets_sharing_pressure(), VisorVariableRecords()).variables assert list(records) == ["POINT::pressure::1"] assert records["POINT::pressure::1"].part_ids == [1, 3] @@ -51,7 +51,8 @@ def test_participation_is_the_union_of_parts_across_datasets(): def test_default_ranges_widen_to_the_min_of_mins_and_max_of_maxes(): """#6: (0.0, 10.0) and (-5.0, 4.0) widen to (-5.0, 10.0); a new id's custom equals its default.""" - record = build_variable_records(_two_datasets_sharing_pressure(), {})["POINT::pressure::1"] + record = VisorVariableRecords.from_registry( + _two_datasets_sharing_pressure(), VisorVariableRecords()).variables["POINT::pressure::1"] assert record.default_magnitude_range == (-5.0, 10.0) assert record.default_ranges == [(-5.0, 10.0)] @@ -68,7 +69,7 @@ def test_a_width_split_produces_distinct_ids(): }) ) - records = build_variable_records(registry, {}) + records = VisorVariableRecords.from_registry(registry, VisorVariableRecords()).variables assert sorted(records) == ["POINT::velocity::1", "POINT::velocity::3"] assert records["POINT::velocity::3"].part_ids == [1] @@ -82,12 +83,13 @@ def _one_dataset_pressure(): def test_an_edited_custom_range_is_kept_across_widening(): """#8: a custom slot that differs from its previous default survives the rebuild.""" - previous = build_variable_records(_one_dataset_pressure(), {}) + previous = VisorVariableRecords.from_registry(_one_dataset_pressure(), VisorVariableRecords()).variables previous["POINT::pressure::1"] = previous["POINT::pressure::1"].model_copy( update={"magnitude_range": (2.0, 3.0), "ranges": [(2.0, 3.0)]} ) - record = build_variable_records(_two_datasets_sharing_pressure(), previous)["POINT::pressure::1"] + record = VisorVariableRecords.from_registry( + _two_datasets_sharing_pressure(), VisorVariableRecords(variables=previous)).variables["POINT::pressure::1"] assert record.default_magnitude_range == (-5.0, 10.0) assert record.magnitude_range == (2.0, 3.0) @@ -96,10 +98,11 @@ def test_an_edited_custom_range_is_kept_across_widening(): def test_an_unedited_custom_range_follows_the_new_default(): """#9: a custom slot equal to its previous default (0.0, 10.0) follows the widening to (-5.0, 10.0).""" - previous = build_variable_records(_one_dataset_pressure(), {}) + previous = VisorVariableRecords.from_registry(_one_dataset_pressure(), VisorVariableRecords()).variables assert previous["POINT::pressure::1"].magnitude_range == (0.0, 10.0) - record = build_variable_records(_two_datasets_sharing_pressure(), previous)["POINT::pressure::1"] + record = VisorVariableRecords.from_registry( + _two_datasets_sharing_pressure(), VisorVariableRecords(variables=previous)).variables["POINT::pressure::1"] assert record.magnitude_range == (-5.0, 10.0) assert record.ranges == [(-5.0, 10.0)] diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index fe66afe8..8e862f5f 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -3283,7 +3283,7 @@ def test_get_state_hands_out_a_copy_of_the_ui_record(scene): # magnitude (0.0, 49.0) and component (10.0, 20.0) on NODE_ID. # =========================================================================== -AGGREGATE_LOGGER = "ansys.visor.viewer.vtk.variables.visor_variable_aggregate.logger" +RECORD_LOGGER = "ansys.visor.viewer.models.common.visor_variable_record.logger" def _pressure_record(**overrides): @@ -3351,7 +3351,7 @@ def test_rebuild_at_remove_drops_ids_with_no_remaining_part(records_scene): 2: _make_part_dataset(2, [SECOND_NODE_ID], part_variables=[ VisorPartVariables(SECOND_NODE_ID, "part", [_temperature_variable()])]), } - records_scene._rebuild_variable_records("add") + records_scene._rebuild_variable_records_from_registry() assert sorted(records_scene._variable_records.variables) == ["CELL::temperature::1", VARIABLE_ID] records_scene.remove_dataset(2) @@ -3365,7 +3365,7 @@ def test_rebuild_at_clear_empties_the_records(records_scene): 1: _make_part_dataset(1, [NODE_ID], part_variables=[ VisorPartVariables(NODE_ID, "part", [_pressure_variable()])]), } - records_scene._rebuild_variable_records("add") + records_scene._rebuild_variable_records_from_registry() holder = records_scene._variable_records records_scene.clear() @@ -3380,13 +3380,13 @@ def test_rebuild_at_update_follows_the_reload_and_the_metadata_refresh(records_s Call-order pin: registry.update_variables (which reloads the part variables), then refresh_descendant_variable_metadata, then the rebuild. """ - from ansys.visor.viewer.vtk.scene import base as base_module + from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecords registry = records_scene._dataset_registry dataset = _make_part_dataset(1, [NODE_ID], part_variables=[ VisorPartVariables(NODE_ID, "part", [_pressure_variable()])]) registry.datasets = {1: dataset} - records_scene._rebuild_variable_records("add") + records_scene._rebuild_variable_records_from_registry() wide_pressure = VisorVariable( index=0, type=VisorVtkVariableType.POINT, name="pressure", num_components=3, @@ -3400,7 +3400,7 @@ def _update_variables(dataset_id, variables): node = records_scene._scene_graph.get_descendant_node.return_value node.refresh_descendant_variable_metadata.side_effect = lambda **_: order.append("refresh") - real_build = base_module.build_variable_records + real_build = VisorVariableRecords.from_registry def _build(*args): order.append("rebuild") @@ -3408,7 +3408,7 @@ def _build(*args): with ( patch.object(registry, "update_variables", side_effect=_update_variables), - patch.object(base_module, "build_variable_records", side_effect=_build), + patch.object(VisorVariableRecords, "from_registry", side_effect=_build), ): records_scene.update_variables_for_dataset(1, []) @@ -3432,13 +3432,13 @@ def test_load_overlays_the_file_range_before_the_part_restore(scene, registry): _restore_part_states -- asserted by recorded order and by the entry the restore sees at the moment it is called. """ - from ansys.visor.viewer.vtk.scene import base as base_module + from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecords _seed_part_variables(registry, [_pressure_variable()]) order = [] seen_by_restore = {} - real_build = base_module.build_variable_records - real_overlay = base_module.overlay_persisted_ranges + real_build = VisorVariableRecords.from_registry + real_overlay = VisorVariableRecords.overlay def _build(*args): order.append("rebuild") @@ -3453,8 +3453,8 @@ def _restore(runtime_app_state): seen_by_restore["entry"] = runtime_app_state.scene.variable_states.get(VARIABLE_ID) with ( - patch.object(base_module, "build_variable_records", side_effect=_build), - patch.object(base_module, "overlay_persisted_ranges", side_effect=_overlay), + patch.object(VisorVariableRecords, "from_registry", side_effect=_build), + patch.object(VisorVariableRecords, "overlay", autospec=True, side_effect=_overlay), patch.object(scene, "_restore_part_states", side_effect=_restore), ): _apply( @@ -3473,7 +3473,7 @@ def test_load_fills_a_null_magnitude_range_with_the_default(scene, registry): """#15: a null magnitudeRange in the file loads at the default, logged at DEBUG.""" _seed_part_variables(registry, [_pressure_variable()]) - with patch(AGGREGATE_LOGGER) as mock_logger: + with patch(RECORD_LOGGER) as mock_logger: _apply(scene, _runtime_state({}), _file_entries(magnitude_range=None, ranges=((3.0, 4.0),))) assert scene._variable_records.variables == { @@ -3491,7 +3491,7 @@ def test_load_drops_a_file_id_with_no_record(scene, registry): num_components=1, magnitude_range=(5.0, 6.0), ranges=[(5.0, 6.0)], ) - with patch(AGGREGATE_LOGGER) as mock_logger: + with patch(RECORD_LOGGER) as mock_logger: _apply(scene, _runtime_state({}), {"POINT::ghost::1": ghost}) assert scene._variable_records.variables == {VARIABLE_ID: _pressure_record()} @@ -3538,7 +3538,7 @@ def _contradictory_reply(): def test_get_state_takes_the_variable_states_from_the_server(scene, registry): """#19: the save receives the server's record, whatever the browser replied.""" _seed_part_variables(registry, [_pressure_variable()]) - scene._rebuild_variable_records("add") + scene._rebuild_variable_records_from_registry() captured = _capture_persist_input(scene, _contradictory_reply()) asyncio.run(scene.get_state(timeout=1.0)) diff --git a/tests/unit/vtk/test_wire_format_identity.py b/tests/unit/vtk/test_wire_format_identity.py index 886ccc42..699eec39 100644 --- a/tests/unit/vtk/test_wire_format_identity.py +++ b/tests/unit/vtk/test_wire_format_identity.py @@ -396,7 +396,7 @@ def _scene_with_two_datasets_sharing_pressure(): }), 2: _VariablesOnlyDataset(2, {21: [_variable("pressure", [(-5.0, 4.0)], (-5.0, 4.0))]}), } - scene._rebuild_variable_records("add") + scene._rebuild_variable_records_from_registry() return scene From 489d37e48086c5bd9995badac5b58fe64353a309 Mon Sep 17 00:00:00 2001 From: Laura Kasian Date: Tue, 29 Sep 2026 10:32:50 -0700 Subject: [PATCH 3/7] feat: clean up server side variable aggregation --- .../viewer/models/common/visor_variable_record.py | 14 ++++++++++++-- src/ansys/visor/viewer/vtk/scene/base.py | 6 +----- tests/unit/vtk/scene/test_base.py | 4 ++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/ansys/visor/viewer/models/common/visor_variable_record.py b/src/ansys/visor/viewer/models/common/visor_variable_record.py index 877a1dc4..ec5f12a4 100644 --- a/src/ansys/visor/viewer/models/common/visor_variable_record.py +++ b/src/ansys/visor/viewer/models/common/visor_variable_record.py @@ -195,8 +195,18 @@ def from_registry( variables[variable_id] = accumulator.to_record(previous.variables.get(variable_id)) return cls(variables=variables) - def overlay(self, file_states: Dict[str, VisorVariableState]) -> "VisorVariableRecords": - """Apply a file's ranges to this fresh build by the D3 load rule and return this holder. + @classmethod + def from_file( + cls, + registry: "VisorDatasetRegistry", + file_states: Dict[str, VisorVariableState], + ) -> "VisorVariableRecords": + """Build a fresh holder from the registry and apply the file's ranges by the D3 load rule.""" + return cls.from_registry(registry, VisorVariableRecords())._overlay(file_states) + + def _overlay(self, file_states: Dict[str, VisorVariableState]) -> "VisorVariableRecords": + """Apply a file's ranges to this fresh build by the D3 load rule and return this holder; called only by + from_file. - A null or absent ``magnitudeRange`` falls back to the default (DEBUG). - ``ranges`` whose length differs from ``num_components`` fall back to the default (DEBUG). diff --git a/src/ansys/visor/viewer/vtk/scene/base.py b/src/ansys/visor/viewer/vtk/scene/base.py index a8b9abb2..77acc9b5 100644 --- a/src/ansys/visor/viewer/vtk/scene/base.py +++ b/src/ansys/visor/viewer/vtk/scene/base.py @@ -568,11 +568,7 @@ def _load_variable_records(self, file_states: Dict[str, VisorVariableState]) -> mutates the live dict or its entries. """ with self._vtk_lock: - records = ( - VisorVariableRecords.from_registry(self._dataset_registry, VisorVariableRecords()) - .overlay(file_states) - .variables - ) + records = VisorVariableRecords.from_file(self._dataset_registry, file_states).variables self._variable_records.variables = records logger.debug("variable records loaded from file: %d records", len(records)) diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index 8e862f5f..ecba9293 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -3438,7 +3438,7 @@ def test_load_overlays_the_file_range_before_the_part_restore(scene, registry): order = [] seen_by_restore = {} real_build = VisorVariableRecords.from_registry - real_overlay = VisorVariableRecords.overlay + real_overlay = VisorVariableRecords._overlay def _build(*args): order.append("rebuild") @@ -3454,7 +3454,7 @@ def _restore(runtime_app_state): with ( patch.object(VisorVariableRecords, "from_registry", side_effect=_build), - patch.object(VisorVariableRecords, "overlay", autospec=True, side_effect=_overlay), + patch.object(VisorVariableRecords, "_overlay", autospec=True, side_effect=_overlay), patch.object(scene, "_restore_part_states", side_effect=_restore), ): _apply( From a3d73d7754538caba3ca7fbe3b673abceb1b7306 Mon Sep 17 00:00:00 2001 From: Laura Kasian Date: Tue, 29 Sep 2026 12:52:19 -0700 Subject: [PATCH 4/7] feat: add set_variable_range trigger and width check --- src/ansys/visor/viewer/app/trame/local_app.py | 31 +- .../models/common/visor_variable_record.py | 15 + .../requests/variable_range_payload.py | 22 ++ src/ansys/visor/viewer/renderer/base.py | 11 + .../visor/viewer/renderer/local_renderer.py | 24 ++ .../visor/viewer/renderer/null_renderer.py | 6 + src/ansys/visor/viewer/vtk/scene/base.py | 288 ++++++++++-------- tests/unit/app/test_local_app.py | 47 ++- tests/unit/vtk/scene/test_base.py | 246 ++++++++++++++- 9 files changed, 544 insertions(+), 146 deletions(-) create mode 100644 src/ansys/visor/viewer/models/runtime/requests/variable_range_payload.py diff --git a/src/ansys/visor/viewer/app/trame/local_app.py b/src/ansys/visor/viewer/app/trame/local_app.py index ce7e7f00..7f1f1885 100644 --- a/src/ansys/visor/viewer/app/trame/local_app.py +++ b/src/ansys/visor/viewer/app/trame/local_app.py @@ -13,6 +13,7 @@ from ansys.visor.viewer.core.visor_logging import VisorDefaultLogger from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.runtime.requests.sync_camera_payload import SyncCameraPayload +from ansys.visor.viewer.models.runtime.requests.variable_range_payload import SetVariableRangePayload from ansys.visor.viewer.models.runtime.requests.widget_state_payloads import ( SetBoundingBoxVisibilityPayload, SetCrossSectionVisibilityPayload, @@ -60,9 +61,11 @@ def set_part_color_variable( association: VisorVtkVariableType, array_name: str, component: int, - min_val: float, - max_val: float, - ) -> None: ... + ) -> bool: ... + + def set_variable_range( + self, variable_id: str, component: int, min_val: float, max_val: float + ) -> bool: ... def clear_part_color_variable(self, node_id: int) -> None: ... @@ -215,6 +218,7 @@ class LocalApp: set_part_selected: selects or deselects one part set_part_color_variable: colours one part by a scalar variable clear_part_color_variable: stops colouring one part by a scalar variable + set_variable_range: stores one variable slot's effective range sync_camera: records a settled camera reported by the frontend set_cross_section_visibility: shows or hides the cross-section plane set_edges_visible: shows or hides edges on every part @@ -444,6 +448,10 @@ def set_part_color_variable(self, payload) -> None: no-op, matching the posture the pipeline takes on an unknown array name. ``variableId`` is forwarded verbatim and is never parsed by the server. + + ``min`` and ``max`` are still required on the wire and appear on the + arrival line, but are not forwarded: the server applies its own + record's effective range for the referenced slot. """ api = self._mutation_api("set_part_color_variable", payload) if api is None: @@ -454,8 +462,6 @@ def set_part_color_variable(self, payload) -> None: payload.association, payload.array_name, payload.component, - payload.min_val, - payload.max_val, ) @trigger("clear_part_color_variable") @@ -467,6 +473,21 @@ def clear_part_color_variable(self, payload) -> None: return api.clear_part_color_variable(payload.node_id) + @trigger("set_variable_range") + @parse_payload(SetVariableRangePayload) + def set_variable_range(self, payload) -> None: + """Frontend -> Backend: store one variable slot's effective range. + + Scene-wide, not per-part: the coordinator applies the range to every + part whose reference names that slot. + """ + api = self._mutation_api("set_variable_range", payload) + if api is None: + return + api.set_variable_range( + payload.variable_id, payload.component, payload.min_val, payload.max_val + ) + # ------------------------------------------------------------------ # Camera trigger # diff --git a/src/ansys/visor/viewer/models/common/visor_variable_record.py b/src/ansys/visor/viewer/models/common/visor_variable_record.py index ec5f12a4..57f0e69b 100644 --- a/src/ansys/visor/viewer/models/common/visor_variable_record.py +++ b/src/ansys/visor/viewer/models/common/visor_variable_record.py @@ -76,6 +76,21 @@ def range_for(self, component: int) -> Range | None: return self.ranges[component] return None + def with_range(self, component: int, value_range: Range) -> "VisorVariableRecord": + """Return a copy with one effective slot replaced: ``-1`` is magnitude, ``0..n-1`` a component. + + The caller validates *component*; a slot outside ``[-1, num_components)`` raises ``IndexError``. + This record is never mutated. + """ + new_range = (float(value_range[0]), float(value_range[1])) + if component == -1: + return self.model_copy(update={"magnitude_range": new_range}, deep=True) + if not 0 <= component < len(self.ranges): + raise IndexError(f"component {component} is outside [-1, {len(self.ranges)})") + ranges = [tuple(r) for r in self.ranges] + ranges[component] = new_range + return self.model_copy(update={"ranges": ranges}, deep=True) + @dataclass class _VariableAccumulator: diff --git a/src/ansys/visor/viewer/models/runtime/requests/variable_range_payload.py b/src/ansys/visor/viewer/models/runtime/requests/variable_range_payload.py new file mode 100644 index 00000000..3f8dfd91 --- /dev/null +++ b/src/ansys/visor/viewer/models/runtime/requests/variable_range_payload.py @@ -0,0 +1,22 @@ +"""Model for the ``set_variable_range`` trigger payload.""" + +from pydantic import BaseModel, ConfigDict, Field + + +class SetVariableRangePayload(BaseModel): + """ + Payload of the ``set_variable_range`` trigger. + + Scene-wide: it names a variable and one of its slots, not a part. + ``component`` uses the server convention, ``-1`` for magnitude and + ``0..n-1`` for a component. Finiteness and ``min <= max`` are checked by + the coordinator, which refuses with a WARNING, not here. + """ + + model_config = ConfigDict(populate_by_name=True) + + variable_id: str = Field(alias="variableId") + component: int + min_val: float = Field(alias="min") + max_val: float = Field(alias="max") + diff --git a/src/ansys/visor/viewer/renderer/base.py b/src/ansys/visor/viewer/renderer/base.py index 27d7096f..a8aafa48 100644 --- a/src/ansys/visor/viewer/renderer/base.py +++ b/src/ansys/visor/viewer/renderer/base.py @@ -155,6 +155,17 @@ def apply_color_variable( def clear_color_variable(self, node_id: int) -> None: """Disable scalar colouring on *node_id*, reverting to solid diffuse.""" + @abstractmethod + def serialize_part_state(self, node_id: int) -> None: + """Make the state served to the client current for one part's mapper. + + A mapper write without this leaves the served cache holding the old + content under a new version number, so the next client fetch gets the + pre-write range. **Serialize only; do not notify.** An unknown + *node_id* is a logged no-op. No-op on a renderer that serves the + client no VTK object state. + """ + @abstractmethod def refresh_color_variable_range( self, diff --git a/src/ansys/visor/viewer/renderer/local_renderer.py b/src/ansys/visor/viewer/renderer/local_renderer.py index 07986440..3e8fb609 100644 --- a/src/ansys/visor/viewer/renderer/local_renderer.py +++ b/src/ansys/visor/viewer/renderer/local_renderer.py @@ -270,6 +270,30 @@ def clear_color_variable(self, node_id: int) -> None: return pipe.clear_color_variable() + def serialize_part_state(self, node_id: int) -> None: + """See :meth:`IRenderer.serialize_part_state`. + + Names the mapper's id alone, derived with ``GetId`` on the object as + :meth:`serialize_camera_state` derives the camera's. An id of ``0`` + is one the store has never held (the ROOT sentinel); naming it + degrades to an error-logged no-op in VTK, so it is skipped here with + a debug line and the next full serialization carries the mapper. + """ + pipe = self._pipelines.get(node_id) + if pipe is None: + logger.debug( + "serialize_part_state: no pipeline for node %s; skipping.", node_id + ) + return + mapper_id = self._object_manager.GetId(pipe.mapper) + if mapper_id == 0: + logger.debug( + "serialize_part_state: mapper of node %s is not registered yet; skipping.", + node_id, + ) + return + self._object_manager.UpdateStateFromObject(mapper_id) + def refresh_color_variable_range( self, node_id: int, diff --git a/src/ansys/visor/viewer/renderer/null_renderer.py b/src/ansys/visor/viewer/renderer/null_renderer.py index 20eaeba7..6dd5e51b 100644 --- a/src/ansys/visor/viewer/renderer/null_renderer.py +++ b/src/ansys/visor/viewer/renderer/null_renderer.py @@ -101,6 +101,12 @@ def apply_color_variable( def clear_color_variable(self, node_id: int) -> None: pass + def serialize_part_state(self, node_id: int) -> None: + """See :meth:`IRenderer.serialize_part_state`. + + No-op: this renderer serves the client no VTK object state. + """ + def refresh_color_variable_range( self, node_id: int, diff --git a/src/ansys/visor/viewer/vtk/scene/base.py b/src/ansys/visor/viewer/vtk/scene/base.py index 77acc9b5..30a80502 100644 --- a/src/ansys/visor/viewer/vtk/scene/base.py +++ b/src/ansys/visor/viewer/vtk/scene/base.py @@ -1,8 +1,10 @@ """VTK scene management for Visor Viewer.""" import json +import math import threading from abc import ABC, abstractmethod +from dataclasses import dataclass from typing import TYPE_CHECKING, Dict, List from trame_server import Server @@ -15,9 +17,13 @@ from ansys.visor.viewer.core.visor_types import VisorDatasetType from ansys.visor.viewer.models.common.visor_camera_state import VisorCameraState from ansys.visor.viewer.models.common.visor_ui_state import VisorUIState -from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecords +from ansys.visor.viewer.models.common.visor_variable_record import ( + VisorVariableRecord, + VisorVariableRecords, +) from ansys.visor.viewer.models.common.visor_variable_state import VisorVariableState from ansys.visor.viewer.models.persist.persisted_viewer_state import PersistedViewerStateV1 +from ansys.visor.viewer.models.runtime.dataset.runtime_dataset_state import RuntimePartProperties from ansys.visor.viewer.models.runtime.visor_scene_details import VisorSceneDetails from ansys.visor.viewer.renderer.base import IRenderer from ansys.visor.viewer.vtk.datasets.visor_dataset import VisorDataset @@ -33,6 +39,35 @@ logger = VisorDefaultLogger(__name__) + +@dataclass(frozen=True) +class _ColorVariableBinding: + """A part's resolved color-variable reference: the record, the slot, and that slot's effective range.""" + + part_id: int + record: VisorVariableRecord + component: int + min_val: float + max_val: float + + def matches(self, association: VisorVtkVariableType, array_name: str) -> bool: + """Return whether *association* and *array_name* name this binding's array.""" + return association is self.record.type and array_name == self.record.array_name + + def apply_to(self, renderer: IRenderer) -> None: + """Configure the part's mapper at this range and re-serialize it; caller holds ``_vtk_lock``.""" + renderer.apply_color_variable( + self.part_id, + self.record.id, + self.record.type, + self.record.array_name, + self.component, + self.min_val, + self.max_val, + ) + renderer.serialize_part_state(self.part_id) + + class VisorSceneBase(ABC): """ Abstract coordinator for a Visor viewer scene. @@ -298,8 +333,8 @@ def apply_state(self, state: PersistedViewerStateV1): runtime_app_state = self._state_mapper.persisted_to_runtime(state) # Variable records: rebuilt from the registry and overlaid with the file's - # ranges, then placed on the runtime state (CC-1), all before - # _restore_part_states, which reads each colored part's range from that dict. + # ranges, then placed on the runtime state for the push, all before + # _restore_part_states, which resolves each colored part against the held record. self._load_variable_records(state.scene.variable_states) runtime_app_state.scene.variable_states = self._variable_records.model_copy(deep=True).variables @@ -743,26 +778,128 @@ def set_part_color_variable( association: VisorVtkVariableType, array_name: str, component: int, - min_val: float, - max_val: float, - ) -> None: + ) -> bool: """ - Colour the part identified by *node_id* by a scalar variable. - - *variable_id* is stored opaquely and is never parsed here; the - association and array name arrive as explicit arguments. *association* - is already a :class:`VisorVtkVariableType` — it is parsed at the - trigger boundary, never derived from a string here. The range travels - as a parameter only and is not persisted per part. + Colour one part at the record's effective range; False, writing nothing, when refused. + + *variable_id* is stored opaquely and is never parsed here. It is + resolved against the held records: the part must participate in it + and *component* must name one of its slots. *association* and + *array_name* must name the record's array. The range applied is the + record's, never the caller's. A refusal logs one WARNING and leaves + both the registry and the mapper untouched. """ with self._vtk_lock: + binding = self._resolve_color_variable(node_id, variable_id, component) + if binding is None: + return False + if not binding.matches(association, array_name): + logger.warning( + "set_part_color_variable: %s '%s' does not name the array of '%s' " + "(part %s); refused.", association, array_name, variable_id, node_id + ) + return False if not self._dataset_registry.set_part_color_variable(node_id, variable_id, component): logger.debug("set_part_color_variable: no dataset owns node %s; skipping.", node_id) - return - self._renderer.apply_color_variable( - node_id, variable_id, association, array_name, component, min_val, max_val + return False + binding.apply_to(self._renderer) + return True + + def set_variable_range( + self, variable_id: str, component: int, min_val: float, max_val: float + ) -> bool: + """ + Store one slot's effective range and apply it to every part referencing that slot; False, writing + nothing, when refused. + + Refused, with a WARNING, for an unknown id, a component outside + ``[-1, num_components)``, a non-finite value, or ``min > max``. The + entry is replaced by copy-on-write and ``variables`` is rebound, so + the unlocked reader never sees the live dict change. Each applied + mapper is re-serialized inside the lock. Nothing is pushed: the + client applied the range before it sent. + """ + with self._vtk_lock: + record = self._variable_records.variables.get(variable_id) + if record is None: + logger.warning("set_variable_range: no record for '%s'; refused.", variable_id) + return False + if not -1 <= component < record.num_components: + logger.warning( + "set_variable_range: component %s is outside [-1, %s) for '%s'; refused.", + component, record.num_components, variable_id + ) + return False + if not (math.isfinite(min_val) and math.isfinite(max_val)): + logger.warning( + "set_variable_range: non-finite range [%s, %s] for '%s'; refused.", + min_val, max_val, variable_id + ) + return False + if min_val > max_val: + logger.warning( + "set_variable_range: min %s is above max %s for '%s'; refused.", + min_val, max_val, variable_id + ) + return False + + variables = dict(self._variable_records.variables) + variables[variable_id] = record.with_range(component, (min_val, max_val)) + self._variable_records.variables = variables + logger.debug( + "variable range stored: %s component %d [%g, %g]", + variable_id, component, min_val, max_val ) + for part_id in variables[variable_id].part_ids: + part_state = self._dataset_registry.get_part_state(part_id) + if part_state is None: + continue + if part_state.variable_id != variable_id or part_state.variable_component != component: + continue + binding = self._resolve_color_variable(part_id, variable_id, component) + if binding is not None: + binding.apply_to(self._renderer) + return True + + def _resolve_color_variable( + self, part_id: int, variable_id: str, component: int + ) -> _ColorVariableBinding | None: + """ + Resolve a part's reference against the held records; None, with one WARNING, when refused. + + Refused when the id has no record, the part does not participate in + it, or *component* names no slot. The id encodes association, name + and width, so a same-named array of another width is another record + and fails participation. Caller holds ``_vtk_lock``. + """ + record = self._variable_records.variables.get(variable_id) + if record is None: + logger.warning( + "_resolve_color_variable: no record for '%s' (part %s); refused.", variable_id, part_id + ) + return None + if part_id not in record.part_ids: + logger.warning( + "_resolve_color_variable: part %s does not participate in '%s'; refused.", + part_id, variable_id + ) + return None + value_range = record.range_for(component) + if value_range is None: + logger.warning( + "_resolve_color_variable: component %s names no slot of '%s' (%s components, part %s); " + "refused.", component, variable_id, record.num_components, part_id + ) + return None + return _ColorVariableBinding( + part_id=part_id, + record=record, + component=component, + min_val=value_range[0], + max_val=value_range[1], + ) + def clear_part_color_variable(self, node_id: int) -> None: """ Stop colouring the part identified by *node_id* by a scalar variable. @@ -893,7 +1030,6 @@ def _restore_part_states(self, runtime_app_state: "RuntimeAppState") -> None: Callers must hold ``_vtk_lock``. """ dataset_states = runtime_app_state.scene.dataset_states or {} - variable_states = runtime_app_state.scene.variable_states or {} self._dataset_registry.replace_part_states(dataset_states) @@ -906,16 +1042,8 @@ def _restore_part_states(self, runtime_app_state: "RuntimeAppState") -> None: ) continue - # Per-part variable metadata, keyed by part id: one entry per - # non-empty leaf. Built once per dataset rather than per part. - variables_by_part = { - entry.part_id: entry.variables for entry in dataset.list_variables() - } - for part_id, part_state in dataset_state.part_states.items(): - self._restore_one_part_state( - part_id, part_state, variable_states, variables_by_part.get(part_id) - ) + self._restore_one_part_state(part_id, part_state) def _restore_camera_state(self, runtime_app_state: "RuntimeAppState") -> None: """ @@ -999,13 +1127,7 @@ def _restore_ui_state(self, runtime_app_state: "RuntimeAppState") -> None: if ui.panel_top_right_tab_index is not None: self._ui_state.panel_top_right_tab_index = ui.panel_top_right_tab_index - def _restore_one_part_state( - self, - part_id: int, - part_state, - variable_states: dict, - part_variables, - ) -> None: + def _restore_one_part_state(self, part_id: int, part_state: RuntimePartProperties) -> None: """ Apply one restored part record to the pipeline. @@ -1044,28 +1166,19 @@ def _restore_one_part_state( ) self._renderer.apply_selected(part_id, part_state.selected, selection_rgb) - self._restore_part_color_variable(part_id, part_state, variable_states, part_variables) + self._restore_part_color_variable(part_id, part_state) - def _restore_part_color_variable( - self, - part_id: int, - part_state, - variable_states: dict, - part_variables, - ) -> None: + def _restore_part_color_variable(self, part_id: int, part_state: RuntimePartProperties) -> None: """ - Restore one part's color-variable reference, or clear it. + Restore one part's color-variable reference through the resolve helper, or clear it. The reference is a compound value, set and cleared as a unit, so if either the identifier or component is missing, it is a logged no-op. - A stored component of ``-1`` reads ``magnitude_range``; 0 or greater - reads ``ranges[component]``. Any other value is refused. - - The array is resolved against the server's per-part variable metadata, - and its width is checked against the stored component count: two parts - can carry same-named arrays of different widths, which the application - treats as different quantities. + Otherwise the reference resolves against the held records, which the + load path has rebuilt and overlaid with the file's ranges before this + runs. A refused reference is a logged no-op; the mapper is re-serialized + after an applied one. """ variable_id = part_state.variable_id component = part_state.variable_component @@ -1087,81 +1200,10 @@ def _restore_part_color_variable( ) return - variable_state = variable_states.get(variable_id) - if variable_state is None: - logger.warning( - "_restore_part_color_variable: no variable entry for '%s' (part %s); skipping.", - variable_id, part_id - ) - return - - if component == -1: - value_range = variable_state.magnitude_range - elif component >= 0: - if component >= len(variable_state.ranges): - logger.warning( - "_restore_part_color_variable: component %s is outside the %s stored ranges " - "for '%s' (part %s); skipping.", - component, len(variable_state.ranges), variable_id, part_id - ) - return - value_range = variable_state.ranges[component] - else: - logger.warning( - "_restore_part_color_variable: component %s for '%s' (part %s) is neither the " - "magnitude sentinel (-1) nor a component index; skipping.", - component, variable_id, part_id - ) + binding = self._resolve_color_variable(part_id, variable_id, component) + if binding is None: return - - if value_range is None: - logger.warning( - "_restore_part_color_variable: no stored range for component %s of '%s' " - "(part %s); skipping.", component, variable_id, part_id - ) - return - - if part_variables is None: - logger.warning( - "_restore_part_color_variable: no variable metadata for part %s; skipping.", - part_id - ) - return - - resolved = next( - ( - variable for variable in part_variables - if variable.name == variable_state.array_name - and variable.type is variable_state.type - ), - None, - ) - if resolved is None: - logger.warning( - "_restore_part_color_variable: array '%s' (%s) not found on part %s; skipping.", - variable_state.array_name, variable_state.type, part_id - ) - return - - if resolved.num_components != variable_state.num_components: - logger.warning( - "_restore_part_color_variable: array '%s' on part %s has %s components, the " - "stored variable has %s; the part does not participate in this variable.", - variable_state.array_name, part_id, - resolved.num_components, variable_state.num_components - ) - return - - min_val, max_val = value_range - self._renderer.apply_color_variable( - part_id, - variable_id, - variable_state.type, - variable_state.array_name, - component, - min_val, - max_val, - ) + binding.apply_to(self._renderer) # ------------------------------------------------------------------ # Internal helpers diff --git a/tests/unit/app/test_local_app.py b/tests/unit/app/test_local_app.py index abf9fb2b..a36ddbf0 100644 --- a/tests/unit/app/test_local_app.py +++ b/tests/unit/app/test_local_app.py @@ -36,6 +36,7 @@ from ansys.visor.viewer.app.trame.local_app import LocalApp from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType +from ansys.visor.viewer.models.runtime.requests.variable_range_payload import SetVariableRangePayload TRIGGER_NAMES = [ "set_part_visibility", @@ -172,7 +173,11 @@ def test_set_part_selected_carries_no_colour(app, api): def test_set_part_color_variable_delegates_payload_values(app, api): - """Every colour-variable field reaches the coordinator in contract order.""" + """Every colour-variable field but the range reaches the coordinator in contract order. + + ``min`` and ``max`` are still required on the wire but are not forwarded: + the server applies its own record's range. + """ app.set_part_color_variable( { "nodeId": 7, @@ -186,7 +191,7 @@ def test_set_part_color_variable_delegates_payload_values(app, api): ) api.set_part_color_variable.assert_called_once_with( - 7, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0, 0.0, 49.0 + 7, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 ) @@ -454,3 +459,41 @@ def test_missing_coordinator_logs_debug_and_not_warning(app_without_api, name): assert mock_logger.warning.call_count == 0 +# =========================================================================== +# set_variable_range +# =========================================================================== + +def test_set_variable_range_payload_parses_wire_aliases(): + """variableId, min and max arrive under their snake_case fields.""" + parsed = SetVariableRangePayload.model_validate( + {"variableId": "POINT::pressure::1", "component": -1, "min": 2.5, "max": 7.5} + ) + + assert parsed.variable_id == "POINT::pressure::1" + assert parsed.component == -1 + assert parsed.min_val == 2.5 + assert parsed.max_val == 7.5 + + +def test_set_variable_range_registered_trigger_delegates_payload_values(app, api, mock_server): + """The registered callable parses the raw dict and forwards all four values.""" + registered = _registered_triggers(mock_server) + + registered["set_variable_range"]( + {"variableId": "POINT::pressure::1", "component": -1, "min": 2.5, "max": 7.5} + ) + + api.set_variable_range.assert_called_once_with("POINT::pressure::1", -1, 2.5, 7.5) + + +def test_set_variable_range_invalid_payload_is_a_logged_no_op(app, api): + """A payload with no max delegates nothing and warns once.""" + with patch("ansys.visor.viewer.app.trame.local_app.logger") as mock_logger: + assert app.set_variable_range( + {"variableId": "POINT::pressure::1", "component": 0, "min": 2.5} + ) is None + + api.set_variable_range.assert_not_called() + assert mock_logger.warning.call_count == 1 + + diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index ecba9293..7bdc7161 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -437,12 +437,48 @@ def test_set_part_selected_flushes_under_the_lock_after_the_apply(scene, pipelin # =========================================================================== # set_part_color_variable +# +# The range applied is the server record's, so each test seeds a literal +# record whose effective ranges differ from every default and from any range +# a client might send. # =========================================================================== +SEEDED_MAGNITUDE_RANGE = (1.5, 8.5) +SEEDED_COMPONENT_RANGE = (3.25, 6.75) +MAPPER_WASM_ID = 8150003 + + +def _seeded_record(**overrides): + """The "pressure" record on NODE_ID with custom ranges distinct from its defaults.""" + fields = dict(magnitude_range=SEEDED_MAGNITUDE_RANGE, ranges=[SEEDED_COMPONENT_RANGE]) + fields.update(overrides) + return _pressure_record(**fields) + + +def _seed_records(scene, *records): + """Install *records* as the scene's held variable records.""" + scene._variable_records.variables = {record.id: record for record in records} + + +def _temperature_record(): + """A cell-association record on NODE_ID for the fixture's "temperature" array.""" + return _pressure_record( + id="CELL::temperature::1", + array_name="temperature", + type=VisorVtkVariableType.CELL, + default_magnitude_range=(0.0, 95.0), + default_ranges=[(0.0, 95.0)], + magnitude_range=(0.0, 95.0), + ranges=[(0.0, 95.0)], + ) + + def test_set_part_color_variable_writes_the_registry_record(scene, registry): """Store half: variable id and component are recorded together.""" + _seed_records(scene, _seeded_record()) + scene.set_part_color_variable( - NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0, 0.0, 49.0 + NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 ) state = registry.get_part_state(NODE_ID) @@ -451,22 +487,26 @@ def test_set_part_color_variable_writes_the_registry_record(scene, registry): def test_set_part_color_variable_applies_to_the_vtk_mapper(scene, pipeline): - """Apply half: the mapper selects the array and honours the given range.""" + """Apply half: the mapper selects the array and honours the record's range.""" + _seed_records(scene, _seeded_record()) + scene.set_part_color_variable( - NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0, 0.0, 49.0 + NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 ) mapper = pipeline.mapper assert mapper.GetScalarVisibility() == 1 assert mapper.GetArrayName() == "pressure" assert mapper.GetArrayComponent() == 0 - assert mapper.GetScalarRange() == pytest.approx((0.0, 49.0)) + assert mapper.GetScalarRange() == pytest.approx((3.25, 6.75)) def test_set_part_color_variable_applies_a_cell_association(scene, pipeline): """Apply half, cell branch: the cell array is selected.""" + _seed_records(scene, _temperature_record()) + scene.set_part_color_variable( - NODE_ID, "CELL::temperature::1", VisorVtkVariableType.CELL, "temperature", 0, 0.0, 95.0 + NODE_ID, "CELL::temperature::1", VisorVtkVariableType.CELL, "temperature", 0 ) assert pipeline.mapper.GetArrayName() == "temperature" @@ -475,11 +515,12 @@ def test_set_part_color_variable_applies_a_cell_association(scene, pipeline): def test_set_part_color_variable_flushes_under_the_lock_after_the_apply(scene, pipeline): """Lock held, and the mapper already configured, when the flush runs.""" + _seed_records(scene, _seeded_record()) scene._vtk_lock = _LockSpy() record = _spy_flush(scene, lambda: pipeline.mapper.GetArrayName()) scene.set_part_color_variable( - NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0, 0.0, 49.0 + NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 ) # TODO: change "calls" to 1 and uncomment "depth" and "value" when we add round trips @@ -489,6 +530,187 @@ def test_set_part_color_variable_flushes_under_the_lock_after_the_apply(scene, p assert scene._vtk_lock.enter_count == scene._vtk_lock.exit_count +def test_set_part_color_variable_applies_the_record_range(scene, pipeline): + """The component slot's effective range comes from the record, not from any caller value.""" + _seed_records(scene, _seeded_record()) + + assert scene.set_part_color_variable( + NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 + ) is True + + assert pipeline.mapper.GetScalarRange() == pytest.approx((3.25, 6.75)) + + +def test_set_part_color_variable_non_participating_part_is_refused_and_not_stored( + scene, registry +): + """A part outside the record's part_ids is refused before the registry is written.""" + _seed_records(scene, _seeded_record(part_ids=[SECOND_NODE_ID])) + + result = scene.set_part_color_variable( + NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 + ) + + assert result is False + assert registry.get_part_state(NODE_ID) is None + + +def test_set_part_color_variable_reserializes_the_mapper_after_the_write_under_the_lock( + scene, pipeline +): + """The mapper is re-serialized by its own id, after the range is written, with the lock held.""" + _seed_records(scene, _seeded_record()) + scene._vtk_lock = _LockSpy() + object_manager = scene._renderer._object_manager + mapper = pipeline.mapper + other_ids = object_manager.GetId.side_effect + object_manager.GetId.side_effect = ( + lambda obj: MAPPER_WASM_ID if obj is mapper else other_ids(obj) + ) + serialized = [] + object_manager.UpdateStateFromObject.side_effect = lambda object_id: serialized.append( + (object_id, scene._vtk_lock.depth, tuple(mapper.GetScalarRange())) + ) + + scene.set_part_color_variable( + NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 + ) + + assert len(serialized) == 1 + object_id, depth, served_range = serialized[0] + assert object_id == 8150003 + assert depth >= 1 + assert served_range == pytest.approx((3.25, 6.75)) + + +# =========================================================================== +# _resolve_color_variable +# =========================================================================== + +def test_resolve_color_variable_magnitude_slot_returns_the_record_magnitude_range(scene): + """Component -1 resolves to the record's magnitude_range, for the part asked about.""" + _seed_records(scene, _seeded_record()) + + binding = scene._resolve_color_variable(NODE_ID, "POINT::pressure::1", -1) + + assert binding.part_id == 7 + assert (binding.min_val, binding.max_val) == (1.5, 8.5) + + +def test_resolve_color_variable_component_slot_returns_the_record_component_range(scene): + """Component 0 resolves to ranges[0], not the magnitude range.""" + _seed_records(scene, _seeded_record()) + + binding = scene._resolve_color_variable(NODE_ID, "POINT::pressure::1", 0) + + assert binding.component == 0 + assert (binding.min_val, binding.max_val) == (3.25, 6.75) + + +def test_resolve_color_variable_component_out_of_range_is_refused(scene): + """A component past the record's width names no slot.""" + _seed_records(scene, _seeded_record()) + + with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: + binding = scene._resolve_color_variable(NODE_ID, "POINT::pressure::1", 1) + + assert binding is None + assert mock_logger.warning.call_count == 1 + + +def test_resolve_color_variable_non_participating_part_is_refused(scene): + """A part not in part_ids is refused; a same-named array of another width lands here.""" + _seed_records(scene, _seeded_record(part_ids=[SECOND_NODE_ID])) + + with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: + binding = scene._resolve_color_variable(NODE_ID, "POINT::pressure::1", 0) + + assert binding is None + assert mock_logger.warning.call_count == 1 + + +def test_apply_state_resolves_through_the_color_variable_helper(scene, registry, pipeline): + """The load path resolves a colored part through the same helper the live trigger uses.""" + _seed_part_variables(registry, [_pressure_variable()]) + + with patch.object( + scene, "_resolve_color_variable", wraps=scene._resolve_color_variable + ) as spy: + _apply(scene, _color_variable_state(0), _file_entries()) + + spy.assert_called_once_with(7, "POINT::pressure::1", 0) + + +# =========================================================================== +# set_variable_range +# =========================================================================== + +def test_set_variable_range_stores_the_slot_by_copy_on_write_without_a_push(scene): + """The slot is replaced in a new dict; a reader's earlier dict is unchanged; nothing is pushed.""" + _seed_records(scene, _seeded_record()) + scene._push_runtime_state = MagicMock(name="_push_runtime_state") + before = scene._variable_records.variables + + assert scene.set_variable_range("POINT::pressure::1", 0, 3.0, 4.0) is True + + stored = scene._variable_records.variables["POINT::pressure::1"] + assert stored.ranges == [(3.0, 4.0)] + assert stored.magnitude_range == (1.5, 8.5) + assert scene._variable_records.variables is not before + assert before["POINT::pressure::1"].ranges == [(3.25, 6.75)] + scene._renderer.flush_wasm_state.assert_not_called() + scene._push_runtime_state.assert_not_called() + + +def test_set_variable_range_applies_to_parts_referencing_the_slot( + scene, registry, pipeline, renderer, array_dataset +): + """A part referencing the slot takes the range; one on another slot of the same id does not.""" + registry.datasets = {1: _make_part_dataset(1, [NODE_ID, SECOND_NODE_ID])} + second = VtkNodePipeline.from_dataset(array_dataset) + renderer._pipelines[SECOND_NODE_ID] = second + _seed_records(scene, _seeded_record(part_ids=[NODE_ID, SECOND_NODE_ID])) + registry.set_part_color_variable(NODE_ID, "POINT::pressure::1", 0) + registry.set_part_color_variable(SECOND_NODE_ID, "POINT::pressure::1", -1) + _seed_unconfigured_mapper(pipeline) + _seed_unconfigured_mapper(second) + + scene.set_variable_range("POINT::pressure::1", 0, 3.0, 4.0) + + assert pipeline.mapper.GetScalarVisibility() == 1 + assert pipeline.mapper.GetScalarRange() == pytest.approx((3.0, 4.0)) + assert second.mapper.GetScalarVisibility() == 0 + assert second.mapper.GetScalarRange() == pytest.approx((11.0, 22.0)) + + +REFUSED_RANGE_CALLS = { + "unknown_id": ("POINT::absent::1", 0, 3.0, 4.0), + "component_out_of_range": ("POINT::pressure::1", 1, 3.0, 4.0), + "min_above_max": ("POINT::pressure::1", 0, 4.0, 3.0), + "non_finite": ("POINT::pressure::1", 0, float("nan"), 4.0), +} + + +@pytest.mark.parametrize("case", list(REFUSED_RANGE_CALLS)) +def test_set_variable_range_refusal_writes_nothing_and_returns_false( + scene, registry, pipeline, case +): + """Refused: False, one WARNING, the held dict not rebound, the referencing mapper untouched.""" + _seed_records(scene, _seeded_record()) + registry.set_part_color_variable(NODE_ID, "POINT::pressure::1", 0) + _seed_unconfigured_mapper(pipeline) + before = scene._variable_records.variables + + with patch("ansys.visor.viewer.vtk.scene.base.logger") as mock_logger: + result = scene.set_variable_range(*REFUSED_RANGE_CALLS[case]) + + assert result is False + assert mock_logger.warning.call_count == 1 + assert scene._variable_records.variables is before + assert pipeline.mapper.GetScalarVisibility() == 0 + assert pipeline.mapper.GetScalarRange() == pytest.approx((11.0, 22.0)) + + # =========================================================================== # clear_part_color_variable # =========================================================================== @@ -506,8 +728,9 @@ def test_clear_part_color_variable_clears_the_registry_record(scene, registry): def test_clear_part_color_variable_disables_scalar_visibility_on_the_mapper(scene, pipeline): """Apply half: scalar colouring is off on the mapper.""" + _seed_records(scene, _seeded_record()) scene.set_part_color_variable( - NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0, 0.0, 49.0 + NODE_ID, "POINT::pressure::1", VisorVtkVariableType.POINT, "pressure", 0 ) scene.clear_part_color_variable(NODE_ID) @@ -538,15 +761,6 @@ def test_clear_part_color_variable_flushes_under_the_lock_after_the_apply(scene, "set_part_opacity": (UNKNOWN_NODE_ID, 0.25), "set_part_diffuse_color": (UNKNOWN_NODE_ID, [1.0, 0.0, 0.0]), "set_part_selected": (UNKNOWN_NODE_ID, True), - "set_part_color_variable": ( - UNKNOWN_NODE_ID, - "POINT::pressure::1", - VisorVtkVariableType.POINT, - "pressure", - 0, - 0.0, - 49.0, - ), "clear_part_color_variable": (UNKNOWN_NODE_ID,), } From 5b0063c443c74b901ea0f9004d1148cf4cbd1665 Mon Sep 17 00:00:00 2001 From: pyansys-ci-bot <92810346+pyansys-ci-bot@users.noreply.github.com> Date: Thu, 1 Oct 2026 18:14:34 +0000 Subject: [PATCH 5/7] chore: adding changelog file 152.added.md [dependabot-skip] --- doc/changelog.d/152.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 doc/changelog.d/152.added.md diff --git a/doc/changelog.d/152.added.md b/doc/changelog.d/152.added.md new file mode 100644 index 00000000..8b1db4ea --- /dev/null +++ b/doc/changelog.d/152.added.md @@ -0,0 +1 @@ +[Remote rendering 3.5a] move variable ownership to server From 29fffdbe449a5ae132e2f91b50218e6efde50b57 Mon Sep 17 00:00:00 2001 From: Laura Kasian Date: Thu, 1 Oct 2026 12:37:42 -0700 Subject: [PATCH 6/7] clean up docs --- .../models/common/visor_variable_record.py | 7 +++---- .../requests/visor_save_state_response.py | 1 - .../models/test_visor_save_state_response.py | 6 ++---- tests/unit/vtk/scene/test_base.py | 17 ++++++++--------- tests/unit/vtk/scene/test_visor_state_mapper.py | 2 +- tests/unit/vtk/test_wire_format_identity.py | 2 +- .../test_visor_variable_empty_block.py | 9 ++++----- 7 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/ansys/visor/viewer/models/common/visor_variable_record.py b/src/ansys/visor/viewer/models/common/visor_variable_record.py index 57f0e69b..afd14db7 100644 --- a/src/ansys/visor/viewer/models/common/visor_variable_record.py +++ b/src/ansys/visor/viewer/models/common/visor_variable_record.py @@ -143,7 +143,7 @@ def add_part(self, part_id: int, variable: "VisorVariable") -> None: self.components[k] = self._widen(self.components[k], variable.ranges[k]) def to_record(self, previous_record: VisorVariableRecord | None) -> VisorVariableRecord: - """Build the record, carrying each custom slot from ``previous_record`` by the D3 rule.""" + """Build the record, carrying each custom slot from ``previous_record``.""" n = self.num_components default_magnitude = self.magnitude or (0.0, 0.0) default_ranges = [slot if slot is not None else (0.0, 0.0) for slot in self.components] @@ -216,12 +216,11 @@ def from_file( registry: "VisorDatasetRegistry", file_states: Dict[str, VisorVariableState], ) -> "VisorVariableRecords": - """Build a fresh holder from the registry and apply the file's ranges by the D3 load rule.""" + """Build a fresh holder from the registry and apply the file's ranges.""" return cls.from_registry(registry, VisorVariableRecords())._overlay(file_states) def _overlay(self, file_states: Dict[str, VisorVariableState]) -> "VisorVariableRecords": - """Apply a file's ranges to this fresh build by the D3 load rule and return this holder; called only by - from_file. + """Apply a file's ranges to this fresh build and return this holder; called only by from_file. - A null or absent ``magnitudeRange`` falls back to the default (DEBUG). - ``ranges`` whose length differs from ``num_components`` fall back to the default (DEBUG). diff --git a/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py b/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py index 9b45ef5b..ceb289b1 100644 --- a/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py +++ b/src/ansys/visor/viewer/models/runtime/requests/visor_save_state_response.py @@ -25,7 +25,6 @@ class VisorSaveStateResponse(BaseModel): app_state: RuntimeAppState = Field(alias="appState") # Guards a client that still sends variableStates in the pre-record shape. - # A later increment decides whether this validator stays. @model_validator(mode="before") @classmethod def _discard_browser_variable_states(cls, data: Any) -> Any: diff --git a/tests/unit/models/test_visor_save_state_response.py b/tests/unit/models/test_visor_save_state_response.py index c241e9dc..6272ff37 100644 --- a/tests/unit/models/test_visor_save_state_response.py +++ b/tests/unit/models/test_visor_save_state_response.py @@ -115,9 +115,7 @@ def test_model_dump_contains_fields(monkeypatch): def test_save_path_rejects_a_variable_state_missing_the_identity_fields(): - """Rewritten in place (3.5.1 increment 1): the save path no longer rejects this entry. - - The server owns the variable records, so the browser's ``variableStates`` is + """ The server owns the variable records, so the browser's ``variableStates`` is discarded before validation. An entry missing the identity fields (an old client) therefore parses, and nothing of it reaches the model. """ @@ -142,7 +140,7 @@ def test_save_path_rejects_a_variable_state_missing_the_identity_fields(): def test_save_path_accepts_a_variable_state_carrying_the_identity_fields(): - """Rewritten in place (3.5.1 increment 1): a complete browser entry is discarded too.""" + """ The server owns the variable records, so the browser's ``variableStates`` is discarded before validation.""" payload = { "requestId": 1, "appState": { diff --git a/tests/unit/vtk/scene/test_base.py b/tests/unit/vtk/scene/test_base.py index 7bdc7161..70324494 100644 --- a/tests/unit/vtk/scene/test_base.py +++ b/tests/unit/vtk/scene/test_base.py @@ -2082,10 +2082,10 @@ def test_apply_state_component_beyond_the_stored_ranges_is_a_logged_no_op( def test_apply_state_absent_range_is_a_logged_no_op(scene, registry, pipeline): - """Rewritten in place (3.5.1 increment 1): an absent file range now loads at the default. + """An absent file range now loads at the default. - The overlay fills a null ``magnitudeRange`` from the record's default - (D3), so the entry the restore reads always carries a range: the part is + The overlay fills a null ``magnitudeRange`` from the record's default, + so the entry the restore reads always carries a range: the part is colored at the widened default (0.0, 49.0) and nothing is refused. """ _seed_part_variables(registry, [_pressure_variable()]) @@ -2104,7 +2104,7 @@ def test_apply_state_unknown_variable_identifier_is_a_logged_no_op( ): """A stored identifier with no record applies nothing. - Rewritten in place (3.5.1 increment 1): the entry now comes from the + The entry comes from the record, which the registry's "pressure" always produces, so the part names an identifier no dataset carries. """ @@ -2121,7 +2121,7 @@ def test_apply_state_unknown_variable_identifier_is_a_logged_no_op( def test_apply_state_unknown_array_name_is_a_logged_no_op(scene, registry, pipeline): """An array the part does not carry applies nothing. - Rewritten in place (3.5.1 increment 1): the record's array name comes from + The record's array name comes from the registry, so "pressure" lives on another part and this part carries only a cell array. """ @@ -2142,7 +2142,7 @@ def test_apply_state_unknown_array_name_is_a_logged_no_op(scene, registry, pipel def test_apply_state_array_width_mismatch_is_a_logged_no_op(scene, registry, pipeline): """Same name and association, different width: a different quantity. - Rewritten in place (3.5.1 increment 1): the width-3 record exists because + The width-3 record exists because another part carries a width-3 "pressure"; this part's is width 1. """ wide_pressure = VisorVariable( @@ -2851,8 +2851,7 @@ def test_get_state_derives_orthographic_enabled_from_the_camera_record(scene): This is the assertion that closes AC-5. The record carries the hand-written literal ``True`` while the reply carries the hand-written literal ``False``; revert the derivation and the assertion reports - ``False``, which is the reply's answer passed through -- the behaviour - before this increment. + ``False``, which is the reply's answer passed through. ``record_reads == 1`` is asserted here too: the record is bound once and read once, so the camera and the projection are answers to a single @@ -3490,7 +3489,7 @@ def test_get_state_hands_out_a_copy_of_the_ui_record(scene): # =========================================================================== -# 3.5.1 increment 1 -- the server's variable records +# The server's variable records # # Rebuild points (#10-#16), the set_state push (#18) and get_state (#19, #20). # Expected values are hand-written literals: the fixture "pressure" variable is diff --git a/tests/unit/vtk/scene/test_visor_state_mapper.py b/tests/unit/vtk/scene/test_visor_state_mapper.py index eeeb1d45..f84abbaa 100644 --- a/tests/unit/vtk/scene/test_visor_state_mapper.py +++ b/tests/unit/vtk/scene/test_visor_state_mapper.py @@ -103,7 +103,7 @@ def fake_from_components(**kwargs): assert called["datasets"] == {"dsA": {"converted": {"state": 123}}} assert called["unit"] == "m" assert called["camera"] == "cam" - # Rewritten in place (3.5.1 increment 1): the record is projected, not passed through. + # The record is projected, not passed through. assert called["variable_states"] == { "POINT::pressure::1": VisorVariableState( id="POINT::pressure::1", diff --git a/tests/unit/vtk/test_wire_format_identity.py b/tests/unit/vtk/test_wire_format_identity.py index 699eec39..263b5035 100644 --- a/tests/unit/vtk/test_wire_format_identity.py +++ b/tests/unit/vtk/test_wire_format_identity.py @@ -351,7 +351,7 @@ def test_data_array_type_is_str_and_point_or_cell_in_emitted_json(): # --------------------------------------------------------------------------- -# 3.5.1 increment 1: the server's variable records on the scene-details wire. +# The server's variable records on the scene-details wire. # # The registry holds two hand-written datasets sharing "pressure"; the scene # graph is the fixture sphere and plays no part in the variables. Expected diff --git a/tests/unit/vtk/variables/test_visor_variable_empty_block.py b/tests/unit/vtk/variables/test_visor_variable_empty_block.py index 4db23d6c..5b2bd725 100644 --- a/tests/unit/vtk/variables/test_visor_variable_empty_block.py +++ b/tests/unit/vtk/variables/test_visor_variable_empty_block.py @@ -1,12 +1,11 @@ -"""G4: variable metadata on a multiblock with an empty (None) block (3.5.1 increment 1, test 23). +"""G4: variable metadata on a multiblock with an empty (None) block. The scene graph and the dataset's PartIndex must name the same parts with the same variables, or the record's ``part_ids`` name ids the client's nodes lack. Expected to fail today on F-E: ``VisorGroupNode._post_init`` walks every ``GetBlock(i)`` including ``None``, and node creation raises RuntimeError. -PartIndex skips empty blocks. Recorded as found-not-fixed; neither -part_index.py nor group_node.py is edited in this increment. +PartIndex skips empty blocks. Found-not-fixed. """ import pytest from vtkmodules.vtkCommonCore import vtkFloatArray @@ -44,8 +43,8 @@ def _multiblock_with_an_empty_block() -> vtkMultiBlockDataSet: @pytest.mark.xfail( strict=True, raises=RuntimeError, - reason="F-E: VisorGroupNode._post_init walks the None block and node creation raises " - "RuntimeError; PartIndex skips it. Found, not fixed, in 3.5.1 increment 1.", + reason="VisorGroupNode._post_init walks the None block and node creation raises " + "RuntimeError; PartIndex skips it. Found, not fixed.", ) def test_part_node_data_arrays_and_list_variables_agree_on_an_empty_block(): """#23: per part id, part_node.data_arrays and dataset.list_variables() name the same arrays.""" From 328ccf1dc0978709dfb47c3a5ef5c7e2e34cb72d Mon Sep 17 00:00:00 2001 From: Laura Kasian Date: Thu, 1 Oct 2026 11:21:28 -0700 Subject: [PATCH 7/7] pre-commit fix --- tests/unit/models/test_visor_variable_record_build.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/models/test_visor_variable_record_build.py b/tests/unit/models/test_visor_variable_record_build.py index 81142253..65327da0 100644 --- a/tests/unit/models/test_visor_variable_record_build.py +++ b/tests/unit/models/test_visor_variable_record_build.py @@ -4,8 +4,8 @@ literal, never recomputed the way the code computes it. """ from ansys.visor.viewer.core.visor_enums import VisorVtkVariableType -from ansys.visor.viewer.vtk.variables.visor_part_variables import VisorPartVariables from ansys.visor.viewer.models.common.visor_variable_record import VisorVariableRecords +from ansys.visor.viewer.vtk.variables.visor_part_variables import VisorPartVariables from ansys.visor.viewer.vtk.variables.visor_variables import VisorVariable POINT = VisorVtkVariableType.POINT