Skip to content

Add a button to make value based selection in 3d clipping tool#479

Merged
nvaytet merged 12 commits intomainfrom
value-clipping
Feb 4, 2026
Merged

Add a button to make value based selection in 3d clipping tool#479
nvaytet merged 12 commits intomainfrom
value-clipping

Conversation

@nvaytet
Copy link
Member

@nvaytet nvaytet commented Aug 7, 2025

Click on the V button to add a value selection.
Screenshot_20250807_220859

This is basically like the threshold gallery example made easier.

Draft because I am not sure we need the added complexity if this can be done quite simply like in the gallery example.

@nvaytet nvaytet marked this pull request as ready for review February 3, 2026 22:10
@nvaytet
Copy link
Member Author

nvaytet commented Feb 3, 2026

This would be one way to implement scipp/essdiffraction#102

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playing around with it and it works nicely 👍

  • Can you update the docs page about plots with value threshold? It seems like we no longer need to show a manual implementation?
  • Please update the tooltip of the button that opens the cutting tool. Currently it says 'Hide/show spatial cutting tool'. But it is no longer only spatial.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the docs page about plots with value threshold? It seems like we no longer need to show a manual implementation?

I think I still want to keep the example in the gallery. Doing it that way is still a valid use case I feel, as it doesn't require interaction from the user to actually get the desired plot.

What I could do though, is a page explaining in more detail how to use the clipping tools though... I opened #523 for this.

Comment on lines 192 to 196
A tool that provides a slider to extract a points in a three-dimensional
scatter plot based on a value selection criterion, and add it to the scene as an
opaque cut. The slider controls the range of the selection.

.. versionadded:: 25.08.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A tool that provides a slider to extract a points in a three-dimensional
scatter plot based on a value selection criterion, and add it to the scene as an
opaque cut. The slider controls the range of the selection.
.. versionadded:: 25.08.0
A tool that provides a slider to extract points in a three-dimensional
scatter plot based on a value selection criterion, and adds it to the scene as an
opaque cut. The slider controls the range of the selection.
.. versionadded:: 25.02.0

How do you handle setting a future version in this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you handle setting a future version in this?

As far as I know, you can't. You have to guess in advance what the version will be 😞

self._update()


class ClippingPlanes(ipw.HBox):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better name? It's not really just planes anymore.

disabled=True,
tooltip='Operation to combine multiple cuts',
layout={'width': '60px', 'padding': '0px 0px 0px 0px'},
layout={'width': '78px', 'padding': '0px 0px 0px 0px'},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be specified in a relative unit to deal with different font sizes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would using em work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know. But why not?

self.layout.display = 'none'

def _add_cut(self, direction: Literal['x', 'y', 'z']):
def _add_cut(self, direction: Literal['x', 'y', 'z', 'v']):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _add_cut(self, direction: Literal['x', 'y', 'z', 'v']):
def _add_cut(self, tool_kind: Literal['x', 'y', 'z', 'v']):

or something similar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I can change direction to kind everywhere.

else:
selections.append(
(da.coords[cut.dim] >= xmin) & (da.coords[cut.dim] < xmax)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this and the branch in _remove_cut above be handled by the tools themselves? I.e., use a common interface and avoid branches here to keep this class simpler.
(In particular since the _remove_cut looks at a private attribute.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion to move this part inside the tool 👍
For the _remove_cut, I kept it as is because:

  1. the tool would then have to have knowledge about the canvas so it can remove itself from it (pythreejs objects have no knowledge of which Scene they are being displayed on, the Scene has to remove them via scene.remove(outline), which is what the canvas is doing internally.
  2. it is actually useful to have a direction attribute to identify the type of clip, and I would be making use of that in Updates to Dream instrument view essdiffraction#242

self._unit = self._limits.unit
self.visible = True
self._update = update
self._direction = 'v'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed if you implement my suggestion for avoiding branches in ClippingPlanes.

@nvaytet nvaytet merged commit cc9d279 into main Feb 4, 2026
4 checks passed
@nvaytet nvaytet deleted the value-clipping branch February 4, 2026 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants