From 47cbf9bcd8ce1cc767ed26bbddc399ef67de61af Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Fri, 5 Sep 2025 15:53:22 +0200 Subject: [PATCH 01/15] if positions have changed, remove point cloud and make a new one --- src/plopp/backends/pythreejs/scatter3d.py | 38 ++++++++++++++++++++--- src/plopp/plotting/scatter3d.py | 14 ++++----- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index af758fc09..5efa68441 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -63,7 +63,7 @@ def __init__( opacity: float = 1, pixel_size: sc.Variable | float | None = None, ): - import pythreejs as p3 + # import pythreejs as p3 check_ndim(data, ndim=1, origin='Scatter3d') self.uid = uid if uid is not None else uuid.uuid4().hex @@ -73,6 +73,7 @@ def __init__( self._x = x self._y = y self._z = z + self._opacity = opacity # TODO: remove pixel_size in the next release self._size = size if pixel_size is None else pixel_size @@ -88,14 +89,27 @@ def __init__( dtype=float, unit=self._data.coords[x].unit ).value + self.points = None + self._make_point_cloud() + if self._colormapper is not None: self._colormapper.add_artist(self.uid, self) - colors = self._colormapper.rgba(self.data)[..., :3].astype('float32') + # colors = self._colormapper.rgba(self.data)[..., :3].astype('float32') + self._update_colors() else: colors = np.broadcast_to( np.array(to_rgb(f'C{artist_number}' if color is None else color)), (self._data.coords[self._x].shape[0], 3), ).astype('float32') + self.geometry.attributes["color"].array = colors + + self._add_point_cloud_to_scene() + + def _make_point_cloud(self): + import pythreejs as p3 + + if self.points is not None: + self._canvas.remove(self.points) self.geometry = p3.BufferGeometry( attributes={ @@ -108,7 +122,11 @@ def __init__( ] ).T ), - 'color': p3.BufferAttribute(array=colors), + 'color': p3.BufferAttribute( + array=np.zeros( + (self._data.coords[self._x].shape[0], 3), dtype='float32' + ) + ), } ) @@ -120,9 +138,11 @@ def __init__( vertexColors='VertexColors', size=2.5 * self._size * pixel_ratio, transparent=True, - opacity=opacity, + opacity=self._opacity, ) self.points = p3.Points(geometry=self.geometry, material=self.material) + + def _add_point_cloud_to_scene(self): self._canvas.add(self.points) def notify_artist(self, message: str) -> None: @@ -155,10 +175,20 @@ def update(self, new_values): New data to update the point cloud values from. """ check_ndim(new_values, ndim=1, origin='Scatter3d') + need_new_point_cloud = any( + not sc.identical(new_values.coords[dim], self._data.coords[dim]) + for dim in [self._x, self._y, self._z] + ) + if need_new_point_cloud: + self._make_point_cloud() + self._data = new_values if self._colormapper is not None: self._update_colors() + if need_new_point_cloud: + self._add_point_cloud_to_scene() + @property def opacity(self) -> float: """ diff --git a/src/plopp/plotting/scatter3d.py b/src/plopp/plotting/scatter3d.py index a3c529462..b431c20cb 100644 --- a/src/plopp/plotting/scatter3d.py +++ b/src/plopp/plotting/scatter3d.py @@ -125,11 +125,11 @@ def scatter3d( camera=camera, **kwargs, ) - clip_planes = ClippingPlanes(fig) - fig.toolbar['cut3d'] = ToggleTool( - callback=clip_planes.toggle_visibility, - icon='layer-group', - tooltip='Hide/show spatial cutting tool', - ) - fig.bottom_bar.add(clip_planes) + # clip_planes = ClippingPlanes(fig) + # fig.toolbar['cut3d'] = ToggleTool( + # callback=clip_planes.toggle_visibility, + # icon='layer-group', + # tooltip='Hide/show spatial cutting tool', + # ) + # fig.bottom_bar.add(clip_planes) return fig From 2f824d7d309cb412d1716e94c5f3ca1c55864fb8 Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Fri, 5 Sep 2025 16:09:52 +0200 Subject: [PATCH 02/15] add option to disable potentially expensive check --- src/plopp/backends/pythreejs/scatter3d.py | 39 ++++++++++++++++------- src/plopp/plotting/scatter3d.py | 14 ++++---- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 5efa68441..e5896563e 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -45,6 +45,11 @@ class Scatter3d: The opacity of the points. pixel_size: The size of the pixels in the plot. Deprecated (use size instead). + disable_position_update: + Whether to check if the coordinates of new data on update have changed. If so, + the positions of the points are re-computed. If not, only the colors are + updated. This check can be expensive for large data sets, so if you are sure + that the coordinates will not change, you can set this to False. """ def __init__( @@ -62,9 +67,8 @@ def __init__( artist_number: int = 0, opacity: float = 1, pixel_size: sc.Variable | float | None = None, + disable_position_update: bool = False, ): - # import pythreejs as p3 - check_ndim(data, ndim=1, origin='Scatter3d') self.uid = uid if uid is not None else uuid.uuid4().hex self._canvas = canvas @@ -74,6 +78,7 @@ def __init__( self._y = y self._z = z self._opacity = opacity + self._position_update = not disable_position_update # TODO: remove pixel_size in the next release self._size = size if pixel_size is None else pixel_size @@ -94,7 +99,6 @@ def __init__( if self._colormapper is not None: self._colormapper.add_artist(self.uid, self) - # colors = self._colormapper.rgba(self.data)[..., :3].astype('float32') self._update_colors() else: colors = np.broadcast_to( @@ -105,7 +109,10 @@ def __init__( self._add_point_cloud_to_scene() - def _make_point_cloud(self): + def _make_point_cloud(self) -> None: + """ + Create the point cloud geometry and material. + """ import pythreejs as p3 if self.points is not None: @@ -142,7 +149,10 @@ def _make_point_cloud(self): ) self.points = p3.Points(geometry=self.geometry, material=self.material) - def _add_point_cloud_to_scene(self): + def _add_point_cloud_to_scene(self) -> None: + """ + Add the point cloud to the canvas scene. + """ self._canvas.add(self.points) def notify_artist(self, message: str) -> None: @@ -168,6 +178,8 @@ def _update_colors(self): def update(self, new_values): """ Update point cloud array with new values. + If the coordinates have changed, the positions of the points are re-computed, + only if ``validate_on_update`` is ``True``. Parameters ---------- @@ -175,17 +187,20 @@ def update(self, new_values): New data to update the point cloud values from. """ check_ndim(new_values, ndim=1, origin='Scatter3d') - need_new_point_cloud = any( - not sc.identical(new_values.coords[dim], self._data.coords[dim]) - for dim in [self._x, self._y, self._z] - ) + need_new_point_cloud = False + if self._position_update: + # This check is potentially expensive to run on every update, so allow + # users to disable it if they are sure that the data is valid. + need_new_point_cloud = any( + not sc.identical(new_values.coords[dim], self._data.coords[dim]) + for dim in [self._x, self._y, self._z] + ) + self._data = new_values + if need_new_point_cloud: self._make_point_cloud() - - self._data = new_values if self._colormapper is not None: self._update_colors() - if need_new_point_cloud: self._add_point_cloud_to_scene() diff --git a/src/plopp/plotting/scatter3d.py b/src/plopp/plotting/scatter3d.py index b431c20cb..a3c529462 100644 --- a/src/plopp/plotting/scatter3d.py +++ b/src/plopp/plotting/scatter3d.py @@ -125,11 +125,11 @@ def scatter3d( camera=camera, **kwargs, ) - # clip_planes = ClippingPlanes(fig) - # fig.toolbar['cut3d'] = ToggleTool( - # callback=clip_planes.toggle_visibility, - # icon='layer-group', - # tooltip='Hide/show spatial cutting tool', - # ) - # fig.bottom_bar.add(clip_planes) + clip_planes = ClippingPlanes(fig) + fig.toolbar['cut3d'] = ToggleTool( + callback=clip_planes.toggle_visibility, + icon='layer-group', + tooltip='Hide/show spatial cutting tool', + ) + fig.bottom_bar.add(clip_planes) return fig From 8209f1c2bbbb1fc854122288e292c5b77e21969b Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Fri, 5 Sep 2025 16:16:42 +0200 Subject: [PATCH 03/15] add tests --- .../pythreejs/pythreejs_scatter3d_test.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/backends/pythreejs/pythreejs_scatter3d_test.py b/tests/backends/pythreejs/pythreejs_scatter3d_test.py index e3ad6a202..77b946227 100644 --- a/tests/backends/pythreejs/pythreejs_scatter3d_test.py +++ b/tests/backends/pythreejs/pythreejs_scatter3d_test.py @@ -124,3 +124,30 @@ def test_update_raises_when_data_is_not_1d(): sc.DimensionError, match='Scatter3d only accepts data with 1 dimension' ): scat.update(da2d) + + +def test_update_with_new_points(): + da = scatter(npoints=500) + scat = Scatter3d(canvas=Canvas(), data=da, x='x', y='y', z='z') + assert scat.points.geometry.attributes['position'].array.shape[0] == 500 + assert scat.points.geometry.attributes['color'].array.shape[0] == 500 + new = scatter(npoints=200) + scat.update(new) + assert scat.points.geometry.attributes['position'].array.shape[0] == 200 + assert scat.points.geometry.attributes['color'].array.shape[0] == 200 + assert sc.identical(scat._data, new) + + +def test_update_with_new_points_skip_position_update(): + da = scatter(npoints=500) + scat = Scatter3d( + canvas=Canvas(), data=da, x='x', y='y', z='z', disable_position_update=True + ) + assert scat.points.geometry.attributes['position'].array.shape[0] == 500 + assert scat.points.geometry.attributes['color'].array.shape[0] == 500 + new = scatter(npoints=200) + scat.update(new) + # This results in points with weird colors so the final state is bad, but it does + # not crash, and is evidence that the position update was skipped. + assert scat.points.geometry.attributes['position'].array.shape[0] == 500 + assert scat.points.geometry.attributes['color'].array.shape[0] == 500 From a853e59d8bb888c0ab078075d2b05b5249f49faf Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Fri, 5 Sep 2025 16:41:15 +0200 Subject: [PATCH 04/15] get rid of expensive check and check shape instead --- src/plopp/backends/pythreejs/scatter3d.py | 35 +++++++++++-------- .../pythreejs/pythreejs_scatter3d_test.py | 17 +-------- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index e5896563e..cd91f051a 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -45,11 +45,6 @@ class Scatter3d: The opacity of the points. pixel_size: The size of the pixels in the plot. Deprecated (use size instead). - disable_position_update: - Whether to check if the coordinates of new data on update have changed. If so, - the positions of the points are re-computed. If not, only the colors are - updated. This check can be expensive for large data sets, so if you are sure - that the coordinates will not change, you can set this to False. """ def __init__( @@ -67,7 +62,6 @@ def __init__( artist_number: int = 0, opacity: float = 1, pixel_size: sc.Variable | float | None = None, - disable_position_update: bool = False, ): check_ndim(data, ndim=1, origin='Scatter3d') self.uid = uid if uid is not None else uuid.uuid4().hex @@ -78,7 +72,6 @@ def __init__( self._y = y self._z = z self._opacity = opacity - self._position_update = not disable_position_update # TODO: remove pixel_size in the next release self._size = size if pixel_size is None else pixel_size @@ -167,7 +160,7 @@ def notify_artist(self, message: str) -> None: """ self._update_colors() - def _update_colors(self): + def _update_colors(self) -> None: """ Set the point cloud's rgba colors: """ @@ -175,6 +168,18 @@ def _update_colors(self): ..., :3 ].astype('float32') + def _update_positions(self) -> None: + """ + Update the point cloud's positions from the data. + """ + self.geometry.attributes["position"].array = np.array( + [ + self._data.coords[self._x].values.astype('float32'), + self._data.coords[self._y].values.astype('float32'), + self._data.coords[self._z].values.astype('float32'), + ] + ).T + def update(self, new_values): """ Update point cloud array with new values. @@ -188,19 +193,19 @@ def update(self, new_values): """ check_ndim(new_values, ndim=1, origin='Scatter3d') need_new_point_cloud = False - if self._position_update: - # This check is potentially expensive to run on every update, so allow - # users to disable it if they are sure that the data is valid. - need_new_point_cloud = any( - not sc.identical(new_values.coords[dim], self._data.coords[dim]) - for dim in [self._x, self._y, self._z] - ) + if self._data.shape != new_values.shape: + need_new_point_cloud = True + self._data = new_values if need_new_point_cloud: self._make_point_cloud() + else: + self._update_positions() + if self._colormapper is not None: self._update_colors() + if need_new_point_cloud: self._add_point_cloud_to_scene() diff --git a/tests/backends/pythreejs/pythreejs_scatter3d_test.py b/tests/backends/pythreejs/pythreejs_scatter3d_test.py index 77b946227..f3ccaff45 100644 --- a/tests/backends/pythreejs/pythreejs_scatter3d_test.py +++ b/tests/backends/pythreejs/pythreejs_scatter3d_test.py @@ -126,7 +126,7 @@ def test_update_raises_when_data_is_not_1d(): scat.update(da2d) -def test_update_with_new_points(): +def test_update_with_different_number_of_points(): da = scatter(npoints=500) scat = Scatter3d(canvas=Canvas(), data=da, x='x', y='y', z='z') assert scat.points.geometry.attributes['position'].array.shape[0] == 500 @@ -136,18 +136,3 @@ def test_update_with_new_points(): assert scat.points.geometry.attributes['position'].array.shape[0] == 200 assert scat.points.geometry.attributes['color'].array.shape[0] == 200 assert sc.identical(scat._data, new) - - -def test_update_with_new_points_skip_position_update(): - da = scatter(npoints=500) - scat = Scatter3d( - canvas=Canvas(), data=da, x='x', y='y', z='z', disable_position_update=True - ) - assert scat.points.geometry.attributes['position'].array.shape[0] == 500 - assert scat.points.geometry.attributes['color'].array.shape[0] == 500 - new = scatter(npoints=200) - scat.update(new) - # This results in points with weird colors so the final state is bad, but it does - # not crash, and is evidence that the position update was skipped. - assert scat.points.geometry.attributes['position'].array.shape[0] == 500 - assert scat.points.geometry.attributes['color'].array.shape[0] == 500 From 8f1a28cc67b9a141a724f169ff3b05cf441e2b9b Mon Sep 17 00:00:00 2001 From: Neil Vaytet <39047984+nvaytet@users.noreply.github.com> Date: Mon, 8 Sep 2025 09:22:16 +0200 Subject: [PATCH 05/15] Update src/plopp/backends/pythreejs/scatter3d.py Co-authored-by: Jan-Lukas Wynen --- src/plopp/backends/pythreejs/scatter3d.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index cd91f051a..3f84f241b 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -192,9 +192,7 @@ def update(self, new_values): New data to update the point cloud values from. """ check_ndim(new_values, ndim=1, origin='Scatter3d') - need_new_point_cloud = False - if self._data.shape != new_values.shape: - need_new_point_cloud = True + need_new_point_cloud = self._data.shape != new_values.shape self._data = new_values From 810c14d60e493d321aa29d06febc0704968a9ded Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Mon, 8 Sep 2025 17:20:20 +0200 Subject: [PATCH 06/15] only update positions if needed --- src/plopp/backends/pythreejs/scatter3d.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 3f84f241b..07e1fb764 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -1,6 +1,5 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2023 Scipp contributors (https://github.com/scipp) - import uuid from typing import Literal @@ -111,6 +110,8 @@ def _make_point_cloud(self) -> None: if self.points is not None: self._canvas.remove(self.points) + self._backup_coords() + self.geometry = p3.BufferGeometry( attributes={ 'position': p3.BufferAttribute( @@ -142,6 +143,16 @@ def _make_point_cloud(self) -> None: ) self.points = p3.Points(geometry=self.geometry, material=self.material) + def _backup_coords(self) -> None: + """ + Backup the current coordinates to be able to detect changes. + """ + self._old_coords = { + self._x: self._data.coords[self._x], + self._y: self._data.coords[self._y], + self._z: self._data.coords[self._z], + } + def _add_point_cloud_to_scene(self) -> None: """ Add the point cloud to the canvas scene. @@ -172,6 +183,12 @@ def _update_positions(self) -> None: """ Update the point cloud's positions from the data. """ + if all( + sc.identical(self._old_coords[dim], self._data.coords[dim]) + for dim in [self._x, self._y, self._z] + ): + return + self._backup_coords() self.geometry.attributes["position"].array = np.array( [ self._data.coords[self._x].values.astype('float32'), @@ -183,8 +200,6 @@ def _update_positions(self) -> None: def update(self, new_values): """ Update point cloud array with new values. - If the coordinates have changed, the positions of the points are re-computed, - only if ``validate_on_update`` is ``True``. Parameters ---------- From 666a0fbf8c1466382e3adbd827cb22e09c5c6569 Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Mon, 8 Sep 2025 17:26:14 +0200 Subject: [PATCH 07/15] use stack instead of np.array().T --- src/plopp/backends/pythreejs/scatter3d.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 07e1fb764..8e56c19a8 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -189,13 +189,14 @@ def _update_positions(self) -> None: ): return self._backup_coords() - self.geometry.attributes["position"].array = np.array( + self.geometry.attributes["position"].array = np.stack( [ self._data.coords[self._x].values.astype('float32'), self._data.coords[self._y].values.astype('float32'), self._data.coords[self._z].values.astype('float32'), - ] - ).T + ], + axis=1, + ) def update(self, new_values): """ From 5ba1867ce216ad2c3383b8ef22cdd95107f775ac Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Mon, 8 Sep 2025 20:53:10 +0200 Subject: [PATCH 08/15] reduce the number of times colors are updated --- src/plopp/backends/pythreejs/scatter3d.py | 35 ++++++++++++++--------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 8e56c19a8..265868d07 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -86,12 +86,12 @@ def __init__( dtype=float, unit=self._data.coords[x].unit ).value - self.points = None + # self.points = None self._make_point_cloud() if self._colormapper is not None: self._colormapper.add_artist(self.uid, self) - self._update_colors() + # self._update_colors() else: colors = np.broadcast_to( np.array(to_rgb(f'C{artist_number}' if color is None else color)), @@ -99,7 +99,8 @@ def __init__( ).astype('float32') self.geometry.attributes["color"].array = colors - self._add_point_cloud_to_scene() + # self._add_point_cloud_to_scene() + self._canvas.add(self.points) def _make_point_cloud(self) -> None: """ @@ -107,8 +108,8 @@ def _make_point_cloud(self) -> None: """ import pythreejs as p3 - if self.points is not None: - self._canvas.remove(self.points) + # if self.points is not None: + # self._canvas.remove(self.points) self._backup_coords() @@ -153,11 +154,11 @@ def _backup_coords(self) -> None: self._z: self._data.coords[self._z], } - def _add_point_cloud_to_scene(self) -> None: - """ - Add the point cloud to the canvas scene. - """ - self._canvas.add(self.points) + # def _add_point_cloud_to_scene(self) -> None: + # """ + # Add the point cloud to the canvas scene. + # """ + # self._canvas.add(self.points) def notify_artist(self, message: str) -> None: """ @@ -175,6 +176,9 @@ def _update_colors(self) -> None: """ Set the point cloud's rgba colors: """ + import time + + print('updating colors', time.time()) self.geometry.attributes["color"].array = self._colormapper.rgba(self.data)[ ..., :3 ].astype('float32') @@ -213,15 +217,20 @@ def update(self, new_values): self._data = new_values if need_new_point_cloud: + old_points = self.points self._make_point_cloud() else: self._update_positions() - if self._colormapper is not None: - self._update_colors() + # if self._colormapper is not None: + # self._update_colors() if need_new_point_cloud: - self._add_point_cloud_to_scene() + # We delay adding the points to the scene until after updating the colors + # so that we avoid first adding empty points to the scene, and then + # performing a color update, which can cause visible flickering. + self._canvas.remove(old_points) + self._canvas.add(self.points) @property def opacity(self) -> float: From f74eb45bd5b2940c3702f7d88e621cf3e3530eac Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Mon, 8 Sep 2025 23:01:52 +0200 Subject: [PATCH 09/15] change logic in updates and use renderer.hold() to avoid flickering on updates --- src/plopp/backends/pythreejs/scatter3d.py | 77 ++++++++++++----------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 265868d07..32ec6a8fa 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -1,5 +1,7 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2023 Scipp contributors (https://github.com/scipp) + +import time import uuid from typing import Literal @@ -71,6 +73,7 @@ def __init__( self._y = y self._z = z self._opacity = opacity + self._trash = None # TODO: remove pixel_size in the next release self._size = size if pixel_size is None else pixel_size @@ -86,20 +89,18 @@ def __init__( dtype=float, unit=self._data.coords[x].unit ).value - # self.points = None self._make_point_cloud() if self._colormapper is not None: self._colormapper.add_artist(self.uid, self) - # self._update_colors() else: colors = np.broadcast_to( np.array(to_rgb(f'C{artist_number}' if color is None else color)), (self._data.coords[self._x].shape[0], 3), ).astype('float32') self.geometry.attributes["color"].array = colors + self._new_colors = None - # self._add_point_cloud_to_scene() self._canvas.add(self.points) def _make_point_cloud(self) -> None: @@ -108,9 +109,6 @@ def _make_point_cloud(self) -> None: """ import pythreejs as p3 - # if self.points is not None: - # self._canvas.remove(self.points) - self._backup_coords() self.geometry = p3.BufferGeometry( @@ -131,6 +129,7 @@ def _make_point_cloud(self) -> None: ), } ) + self._new_positions = None # TODO: a device pixel_ratio should probably be read from a config file pixel_ratio = 1.0 @@ -154,12 +153,6 @@ def _backup_coords(self) -> None: self._z: self._data.coords[self._z], } - # def _add_point_cloud_to_scene(self) -> None: - # """ - # Add the point cloud to the canvas scene. - # """ - # self._canvas.add(self.points) - def notify_artist(self, message: str) -> None: """ Receive notification from the colormapper that its state has changed. @@ -170,18 +163,12 @@ def notify_artist(self, message: str) -> None: message: The message from the colormapper. """ - self._update_colors() - - def _update_colors(self) -> None: - """ - Set the point cloud's rgba colors: - """ - import time + self._new_colors = self._colormapper.rgba(self.data)[..., :3].astype('float32') - print('updating colors', time.time()) - self.geometry.attributes["color"].array = self._colormapper.rgba(self.data)[ - ..., :3 - ].astype('float32') + # Second call to finalize: this will be the final call if there is a + # colormapper. The first call was made by update() once the positions were + # updated. + self._finalize_update(2) def _update_positions(self) -> None: """ @@ -193,7 +180,7 @@ def _update_positions(self) -> None: ): return self._backup_coords() - self.geometry.attributes["position"].array = np.stack( + return np.stack( [ self._data.coords[self._x].values.astype('float32'), self._data.coords[self._y].values.astype('float32'), @@ -212,25 +199,41 @@ def update(self, new_values): New data to update the point cloud values from. """ check_ndim(new_values, ndim=1, origin='Scatter3d') - need_new_point_cloud = self._data.shape != new_values.shape - + old_shape = self._data.shape self._data = new_values - if need_new_point_cloud: - old_points = self.points + if self._data.shape != old_shape: + self._trash = self.points self._make_point_cloud() else: - self._update_positions() + self._trash = None + self._new_positions = self._update_positions() - # if self._colormapper is not None: - # self._update_colors() + # First call to finalize: this will be the final call if there is no + # colormapper. If there is a colormapper, it will call finalize again once it has + # updated the colors. + self._finalize_update(1) - if need_new_point_cloud: - # We delay adding the points to the scene until after updating the colors - # so that we avoid first adding empty points to the scene, and then - # performing a color update, which can cause visible flickering. - self._canvas.remove(old_points) - self._canvas.add(self.points) + def _finalize_update(self, count: int) -> None: + """ + Finalize the update of the point cloud. + This is called twice if there is a colormapper: + once when the positions are updated, and once when the colors are updated. + We want to wait for both to be ready before updating the geometry. + """ + if count < (1 + bool(self._colormapper is not None)): + return + with self._canvas.renderer.hold(): + if self._new_positions is not None: + self.geometry.attributes["position"].array = self._new_positions + self._new_positions = None + if self._new_colors is not None: + self.geometry.attributes["color"].array = self._new_colors + self._new_colors = None + if self._trash is not None: + self._canvas.remove(self._trash) + self._trash = None + self._canvas.add(self.points) @property def opacity(self) -> float: From a024c3821b1b3bd7a8d81b6796f42da99d8e33ce Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Mon, 8 Sep 2025 23:02:52 +0200 Subject: [PATCH 10/15] formatting --- src/plopp/backends/pythreejs/scatter3d.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 32ec6a8fa..5322f74c1 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -1,7 +1,5 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2023 Scipp contributors (https://github.com/scipp) - -import time import uuid from typing import Literal @@ -210,8 +208,8 @@ def update(self, new_values): self._new_positions = self._update_positions() # First call to finalize: this will be the final call if there is no - # colormapper. If there is a colormapper, it will call finalize again once it has - # updated the colors. + # colormapper. If there is a colormapper, it will call finalize again once it + # has updated the colors. self._finalize_update(1) def _finalize_update(self, count: int) -> None: From 90fbccd10b706c775b4c3a76bdff18dbc101f98a Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Tue, 9 Sep 2025 08:34:08 +0200 Subject: [PATCH 11/15] remove idiotic mechanism with count in finalize function --- src/plopp/backends/pythreejs/scatter3d.py | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 5322f74c1..ee29428cd 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -162,11 +162,7 @@ def notify_artist(self, message: str) -> None: The message from the colormapper. """ self._new_colors = self._colormapper.rgba(self.data)[..., :3].astype('float32') - - # Second call to finalize: this will be the final call if there is a - # colormapper. The first call was made by update() once the positions were - # updated. - self._finalize_update(2) + self._finalize_update() def _update_positions(self) -> None: """ @@ -207,20 +203,16 @@ def update(self, new_values): self._trash = None self._new_positions = self._update_positions() - # First call to finalize: this will be the final call if there is no - # colormapper. If there is a colormapper, it will call finalize again once it - # has updated the colors. - self._finalize_update(1) + if self._colormapper is None: + self._finalize_update() - def _finalize_update(self, count: int) -> None: + def _finalize_update(self) -> None: """ Finalize the update of the point cloud. - This is called twice if there is a colormapper: - once when the positions are updated, and once when the colors are updated. + This is called either at the end of the position update if there is no + colormapper, and after the colors are updated in the case of a colormapper. We want to wait for both to be ready before updating the geometry. """ - if count < (1 + bool(self._colormapper is not None)): - return with self._canvas.renderer.hold(): if self._new_positions is not None: self.geometry.attributes["position"].array = self._new_positions From c8be01c2097475873ed7f4e3349c8b540d8cb57c Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Tue, 9 Sep 2025 09:37:23 +0200 Subject: [PATCH 12/15] improved logic and improved tests --- src/plopp/backends/pythreejs/scatter3d.py | 95 +++++++++++++------ .../pythreejs/pythreejs_scatter3d_test.py | 47 ++++++--- 2 files changed, 101 insertions(+), 41 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index ee29428cd..5fe4d48db 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -1,7 +1,8 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2023 Scipp contributors (https://github.com/scipp) + import uuid -from typing import Literal +from typing import Any, Literal import numpy as np import scipp as sc @@ -70,8 +71,10 @@ def __init__( self._x = x self._y = y self._z = z + self._unique_color = to_rgb(f'C{artist_number}' if color is None else color) self._opacity = opacity - self._trash = None + self._new_points = None + self._new_colors = None # TODO: remove pixel_size in the next release self._size = size if pixel_size is None else pixel_size @@ -87,19 +90,11 @@ def __init__( dtype=float, unit=self._data.coords[x].unit ).value - self._make_point_cloud() + self.points = self._make_point_cloud() + self._canvas.add(self.points) if self._colormapper is not None: self._colormapper.add_artist(self.uid, self) - else: - colors = np.broadcast_to( - np.array(to_rgb(f'C{artist_number}' if color is None else color)), - (self._data.coords[self._x].shape[0], 3), - ).astype('float32') - self.geometry.attributes["color"].array = colors - self._new_colors = None - - self._canvas.add(self.points) def _make_point_cloud(self) -> None: """ @@ -109,21 +104,23 @@ def _make_point_cloud(self) -> None: self._backup_coords() - self.geometry = p3.BufferGeometry( + geometry = p3.BufferGeometry( attributes={ 'position': p3.BufferAttribute( - array=np.array( + array=np.stack( [ self._data.coords[self._x].values.astype('float32'), self._data.coords[self._y].values.astype('float32'), self._data.coords[self._z].values.astype('float32'), - ] - ).T + ], + axis=1, + ) ), 'color': p3.BufferAttribute( - array=np.zeros( - (self._data.coords[self._x].shape[0], 3), dtype='float32' - ) + array=np.broadcast_to( + np.array(self._unique_color), + (self._data.coords[self._x].shape[0], 3), + ).astype('float32') ), } ) @@ -133,13 +130,13 @@ def _make_point_cloud(self) -> None: pixel_ratio = 1.0 # Note that an additional factor of 2.5 (obtained from trial and error) seems to # be required to get the sizes right in the scene. - self.material = p3.PointsMaterial( + material = p3.PointsMaterial( vertexColors='VertexColors', size=2.5 * self._size * pixel_ratio, transparent=True, opacity=self._opacity, ) - self.points = p3.Points(geometry=self.geometry, material=self.material) + return p3.Points(geometry=geometry, material=material) def _backup_coords(self) -> None: """ @@ -197,10 +194,9 @@ def update(self, new_values): self._data = new_values if self._data.shape != old_shape: - self._trash = self.points - self._make_point_cloud() + self._new_points = self._make_point_cloud() else: - self._trash = None + self._new_points = None self._new_positions = self._update_positions() if self._colormapper is None: @@ -213,17 +209,56 @@ def _finalize_update(self) -> None: colormapper, and after the colors are updated in the case of a colormapper. We want to wait for both to be ready before updating the geometry. """ + # We use the hold context manager to avoid multiple re-draws of the scene and + # thus prevent flickering. with self._canvas.renderer.hold(): + if self._new_points is not None: + self._canvas.remove(self.points) + self.points = self._new_points + self._new_points = None + self._canvas.add(self.points) if self._new_positions is not None: - self.geometry.attributes["position"].array = self._new_positions + self.position = self._new_positions self._new_positions = None if self._new_colors is not None: - self.geometry.attributes["color"].array = self._new_colors + self.color = self._new_colors self._new_colors = None - if self._trash is not None: - self._canvas.remove(self._trash) - self._trash = None - self._canvas.add(self.points) + + @property + def position(self) -> np.ndarray: + """ + The scatter points positions as a (N, 3) numpy array. + """ + return self.geometry.attributes['position'].array + + @position.setter + def position(self, val: np.ndarray): + self.geometry.attributes['position'].array = val + + @property + def color(self) -> np.ndarray: + """ + The scatter points colors as a (N, 3) numpy array. + """ + return self.geometry.attributes['color'].array + + @color.setter + def color(self, val: np.ndarray): + self.geometry.attributes['color'].array = val + + @property + def geometry(self) -> Any: + """ + The scatter points geometry. + """ + return self.points.geometry + + @property + def material(self) -> Any: + """ + The scatter points material. + """ + return self.points.material @property def opacity(self) -> float: diff --git a/tests/backends/pythreejs/pythreejs_scatter3d_test.py b/tests/backends/pythreejs/pythreejs_scatter3d_test.py index f3ccaff45..257c49752 100644 --- a/tests/backends/pythreejs/pythreejs_scatter3d_test.py +++ b/tests/backends/pythreejs/pythreejs_scatter3d_test.py @@ -8,15 +8,17 @@ from plopp.backends.pythreejs.canvas import Canvas from plopp.backends.pythreejs.scatter3d import Scatter3d from plopp.data.testing import scatter +from plopp.graphics import ColorMapper -def test_creation(): +@pytest.mark.parametrize("with_cbar", [False, True]) +def test_creation(with_cbar): da = scatter() - scat = Scatter3d(canvas=Canvas(), data=da, x='x', y='y', z='z') + canvas = Canvas() + cmapper = ColorMapper(canvas=canvas) if with_cbar else None + scat = Scatter3d(canvas=canvas, data=da, x='x', y='y', z='z', colormapper=cmapper) assert sc.identical(scat._data, da) - assert np.allclose( - scat.geometry.attributes['position'].array, da.coords['position'].values - ) + assert np.allclose(scat.position, da.coords['position'].values) def test_update(): @@ -126,13 +128,36 @@ def test_update_raises_when_data_is_not_1d(): scat.update(da2d) -def test_update_with_different_number_of_points(): +@pytest.mark.parametrize("with_cbar", [False, True]) +def test_update_with_new_positions(with_cbar): + canvas = Canvas() + cmapper = ColorMapper(canvas=canvas) if with_cbar else None + da = scatter(npoints=500, seed=10) + scat = Scatter3d(canvas=canvas, data=da, x='x', y='y', z='z', colormapper=cmapper) + assert scat.position.shape[0] == 500 + assert scat.color.shape[0] == 500 + new = scatter(npoints=500, seed=20) + new.data = da.data # Keep the same data values + scat.update(new) + if with_cbar: + scat.notify_artist("") # To update the colors + assert scat.position.shape[0] == 500 + assert scat.color.shape[0] == 500 + assert sc.identical(scat._data, new) + + +@pytest.mark.parametrize("with_cbar", [False, True]) +def test_update_with_different_number_of_points(with_cbar): + canvas = Canvas() + cmapper = ColorMapper(canvas=canvas) if with_cbar else None da = scatter(npoints=500) - scat = Scatter3d(canvas=Canvas(), data=da, x='x', y='y', z='z') - assert scat.points.geometry.attributes['position'].array.shape[0] == 500 - assert scat.points.geometry.attributes['color'].array.shape[0] == 500 + scat = Scatter3d(canvas=canvas, data=da, x='x', y='y', z='z', colormapper=cmapper) + assert scat.position.shape[0] == 500 + assert scat.color.shape[0] == 500 new = scatter(npoints=200) scat.update(new) - assert scat.points.geometry.attributes['position'].array.shape[0] == 200 - assert scat.points.geometry.attributes['color'].array.shape[0] == 200 + if with_cbar: + scat.notify_artist("") # To update the colors + assert scat.position.shape[0] == 200 + assert scat.color.shape[0] == 200 assert sc.identical(scat._data, new) From 85249025b7b35fac3d7fd47ca4a7cde52410ba17 Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Tue, 9 Sep 2025 20:50:11 +0200 Subject: [PATCH 13/15] notify artists also when autoscale is false --- src/plopp/graphics/graphicalview.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/plopp/graphics/graphicalview.py b/src/plopp/graphics/graphicalview.py index 4b28c6b1b..8c5e5f299 100644 --- a/src/plopp/graphics/graphicalview.py +++ b/src/plopp/graphics/graphicalview.py @@ -220,6 +220,8 @@ def update(self, *args, **kwargs) -> None: if self._autoscale: self.fit_to_data() + elif self.colormapper is not None: + self.colormapper.notify_artists() self.canvas.draw() From 3e33713578b9206beb590b5337fc626734b6c2dc Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Wed, 10 Sep 2025 09:19:32 +0200 Subject: [PATCH 14/15] change 3d clipping mechanism to use static nodes instead of dynamically adding and removing them --- src/plopp/backends/pythreejs/scatter3d.py | 11 +++++-- src/plopp/widgets/clip3d.py | 39 +++++++++++++++++++++-- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/plopp/backends/pythreejs/scatter3d.py b/src/plopp/backends/pythreejs/scatter3d.py index 5fe4d48db..30398dbbe 100644 --- a/src/plopp/backends/pythreejs/scatter3d.py +++ b/src/plopp/backends/pythreejs/scatter3d.py @@ -135,6 +135,7 @@ def _make_point_cloud(self) -> None: size=2.5 * self._size * pixel_ratio, transparent=True, opacity=self._opacity, + depthTest=self._opacity > 0.5, ) return p3.Points(geometry=geometry, material=material) @@ -215,14 +216,17 @@ def _finalize_update(self) -> None: if self._new_points is not None: self._canvas.remove(self.points) self.points = self._new_points - self._new_points = None - self._canvas.add(self.points) if self._new_positions is not None: self.position = self._new_positions self._new_positions = None if self._new_colors is not None: self.color = self._new_colors self._new_colors = None + # For some reason, adding the points to the scene before updating the colors + # still shows the old colors for a brief moment, even if hold() is active. + if self._new_points is not None: + self._new_points = None + self._canvas.add(self.points) @property def position(self) -> np.ndarray: @@ -265,10 +269,11 @@ def opacity(self) -> float: """ The scatter points opacity. """ - return self.material.opacity + return self._opacity @opacity.setter def opacity(self, val: float): + self._opacity = val self.material.opacity = val self.material.depthTest = val > 0.5 diff --git a/src/plopp/widgets/clip3d.py b/src/plopp/widgets/clip3d.py index cb414ab3a..8f3b60a46 100644 --- a/src/plopp/widgets/clip3d.py +++ b/src/plopp/widgets/clip3d.py @@ -10,7 +10,7 @@ import numpy as np import scipp as sc -from ..core import Node +from ..core import Node, node from ..graphics import BaseFig from .debounce import debounce from .style import BUTTON_LAYOUT @@ -228,7 +228,7 @@ def __init__(self, fig: BaseFig): self.tabs = ipw.Tab(layout={'width': '550px'}) self._original_nodes = list(self._view.graph_nodes.values()) - self._nodes = {} + # self._nodes = {} self.add_cut_label = ipw.Label('Add cut:') layout = {'width': '45px', 'padding': '0px 0px 0px 0px'} @@ -295,6 +295,17 @@ def __init__(self, fig: BaseFig): ) self.delete_cut.on_click(self._remove_cut) + self._nodes = {} + self._cut_info_node = Node(self._get_visible_cuts) + for n in self._original_nodes: + self._nodes[n.id] = Node( + self._select_subset, da=n, cuts=self._cut_info_node + ) + self._nodes[n.id].add_view(self._view) + print("added node", self._nodes[n.id]) + print("view nodes", self._view.graph_nodes) + self.update_state() + super().__init__( [ self.tabs, @@ -354,6 +365,10 @@ def update_controls(self): self.opacity.disabled = not at_least_one_cut opacity = self.opacity.value if at_least_one_cut else 1.0 self._set_opacity({'new': opacity}) + # if not at_least_one_cut: + for n in self._original_nodes: + nid = self._nodes[n.id].id + self._view.artists[nid].visible = at_least_one_cut def _set_opacity(self, change: dict[str, Any]): """ @@ -382,6 +397,24 @@ def change_operation(self, change: dict[str, Any]): self._operation = change['new'].lower() self.update_state() + def _get_visible_cuts(self): + return [cut for cut in self.cuts if cut.visible] + + def _select_subset(self, da, cuts): + selections = [] + npoints = 0 + for cut in cuts: + xmin, xmax = cut.range + selection = (da.coords[cut.dim] >= xmin) & (da.coords[cut.dim] < xmax) + npoints += selection.sum().value + selections.append(selection) + # If no points are selected, return a dummy selection to avoid issues with + # empty selections. + if npoints == 0: + return da[0:1] + sel = OPERATIONS[self._operation](selections) + return da[sel] + def update_state(self): """ Update the state, combining all the active cuts, using the selected binary @@ -392,6 +425,8 @@ def update_state(self): debounce mechanism to avoid updating the cloud too often. Only the outlines of the cuts are moved in real time, which is cheap. """ + self._cut_info_node.notify_children("") + return for nodes in self._nodes.values(): self._view.remove(nodes['slice'].id) nodes['slice'].remove() From c521ef466384a0c0265a0ea883a0a0775d8b7212 Mon Sep 17 00:00:00 2001 From: Neil Vaytet Date: Wed, 10 Sep 2025 10:18:33 +0200 Subject: [PATCH 15/15] cleanup --- src/plopp/widgets/clip3d.py | 68 ++++++++++++------------------------- 1 file changed, 22 insertions(+), 46 deletions(-) diff --git a/src/plopp/widgets/clip3d.py b/src/plopp/widgets/clip3d.py index 8f3b60a46..00626e31b 100644 --- a/src/plopp/widgets/clip3d.py +++ b/src/plopp/widgets/clip3d.py @@ -10,7 +10,7 @@ import numpy as np import scipp as sc -from ..core import Node, node +from ..core import Node from ..graphics import BaseFig from .debounce import debounce from .style import BUTTON_LAYOUT @@ -28,10 +28,6 @@ def _xor(x: list[sc.Variable]) -> sc.Variable: } -def select(da: sc.DataArray, s: tuple[str, sc.Variable]) -> sc.DataArray: - return da[s] - - class Clip3dTool(ipw.HBox): """ A tool that provides a slider to extract a slab of points in a three-dimensional @@ -191,6 +187,16 @@ class ClippingPlanes(ipw.HBox): """ A widget to make clipping planes for spatial cutting (see :class:`Clip3dTool`) to make spatial cuts in the X, Y, and Z directions on a three-dimensional scatter plot. + The widget provides buttons to add/remove cuts, toggle the visibility of the cuts, + and set the operation to combine multiple cuts (OR, AND, XOR). The opacity of the + original point clouds is reduced when at least one cut is active, to provide + context. + + The selection from all cuts are combined to either create or update a + second point cloud which is included in the scene. + When the position/range of a cut is changed, only the outlines of + the cuts are moved in real time, which is cheap. The actual point cloud gets + updated less frequently using a debounce mechanism. .. versionadded:: 24.04.0 @@ -302,8 +308,6 @@ def __init__(self, fig: BaseFig): self._select_subset, da=n, cuts=self._cut_info_node ) self._nodes[n.id].add_view(self._view) - print("added node", self._nodes[n.id]) - print("view nodes", self._view.graph_nodes) self.update_state() super().__init__( @@ -397,10 +401,17 @@ def change_operation(self, change: dict[str, Any]): self._operation = change['new'].lower() self.update_state() - def _get_visible_cuts(self): + def _get_visible_cuts(self) -> list[Clip3dTool]: + """ + Return the list of visible cuts. + """ return [cut for cut in self.cuts if cut.visible] - def _select_subset(self, da, cuts): + def _select_subset(self, da: sc.DataArray, cuts: list[Clip3dTool]) -> sc.DataArray: + """ + Return the subset of the data array selected by the cuts, combined using the + selected operation. + """ selections = [] npoints = 0 for cut in cuts: @@ -417,42 +428,7 @@ def _select_subset(self, da, cuts): def update_state(self): """ - Update the state, combining all the active cuts, using the selected binary - operation. The resulting selection is then used to either create or update a - second point cloud which is included in the scene. - The original point cloud is then set to be semi-transparent. - When the position/range of a cut is changed, this function is called via a - debounce mechanism to avoid updating the cloud too often. Only the outlines of - the cuts are moved in real time, which is cheap. + Update the state of the cuts in the figure by triggering the node that + provides the list of visible cuts. """ self._cut_info_node.notify_children("") - return - for nodes in self._nodes.values(): - self._view.remove(nodes['slice'].id) - nodes['slice'].remove() - self._nodes.clear() - - visible_cuts = [cut for cut in self.cuts if cut.visible] - if not visible_cuts: - return - - for n in self._original_nodes: - da = n.request_data() - selections = [] - for cut in visible_cuts: - xmin, xmax = cut.range - selections.append( - (da.coords[cut.dim] >= xmin) & (da.coords[cut.dim] < xmax) - ) - selection = OPERATIONS[self._operation](selections) - if selection.sum().value > 0: - if n.id not in self._nodes: - select_node = Node(selection) - self._nodes[n.id] = { - 'select': select_node, - 'slice': Node(lambda da, s: da[s], da=n, s=select_node), - } - self._nodes[n.id]['slice'].add_view(self._view) - else: - self._nodes[n.id]['select'].func = lambda: selection # noqa: B023 - self._nodes[n.id]['select'].notify_children("")