Implements limiting size for gridlines #2504

soraxas · 2023-06-24T12:08:42Z

This closes #2311

Test:

from vispy import scene, app

canvas = scene.SceneCanvas(keys="interactive", show=True)

view = canvas.central_widget.add_view()
view.camera = "turntable"

grid_lines = scene.visuals.GridLines(parent=view.scene)

app.run()

Custom limits and boarder width

grid_lines.bounds = -500, 0, 10, 200
grid_lines.boarder_width = 10

Equivalent of infinite grid

grid_lines.bounds = -np.inf, np.inf, -np.inf, np.inf

Signed-off-by: Tin Lai <oscar@tinyiu.com>

djhoese

Wow, very cool. I'm glad the end result doesn't actually involve too much complex code. Besides "border" versus "boarder", could we make the default behavior equivalent to the existing infinite behavior?

Otherwise, we'll need tests for this to make sure all of this stuff works. We don't need to compare the final rendered result with a static image, but we should at least make sure that these new options don't fail. We could actually check that the edges of the rendered result are black when bounds are defined.

vispy/visuals/gridlines.py

Signed-off-by: Tin Lai <oscar@tinyiu.com>

soraxas · 2023-06-25T08:29:45Z

Thanks for pointing out the typo and yep the previous infinite grid is set to be its default behaviour

djhoese · 2023-06-25T19:16:48Z

vispy/visuals/gridlines.py

@@ -86,6 +109,30 @@ def __init__(self, scale=(1, 1), color='w'):
        self.shared_program.frag['get_data'] = self._grid_color_fn
        cfun = Function('vec4 null(vec4 x) { return x; }')
        self.shared_program.frag['color_transform'] = cfun
+        self.unfreeze()
+        self.bounds = bounds


It appears the website building is failing on running an exampel because bounds is already used at the higher levels of vispy. We may need to come up with a better name for this keyword and property.

huh I see, that's strange, does that mans GridLines is used as a nested visual, and the call to x.bounds(...) somehow get passed to GridLines?

hmm... perhaps we can just literally name it as gridline_bounds? (not too creative i know...)

Visual classes are wrapped/subclassed with other classes to work with the SceneCanvas, but it looks like I was wrong about that being the issue. Turns out the BaseVisual class has a bounds method:

vispy/vispy/visuals/visual.py

Lines 241 to 255 in 664460a

def bounds(self, axis, view=None):

"""Get the bounds of the Visual

Parameters

----------

axis : int

The axis.

view : instance of VisualView

The view to use.

"""

if view is None:

view = self

if axis not in self._vshare.bounds:

self._vshare.bounds[axis] = self._compute_bounds(axis, view)

return self._vshare.bounds[axis]

Yeah, I don't like gridline_bounds. grid_bounds maybe, but that's a last resort. Can we come up with anything better? extents? Eh that's kind of weird.

Ah yes that's quite an oversight from my part, as this happens to be methods in all base visual.

Hmmm how about borrowing the terminology from matplotlib of xlimand ylim? (might need to split them into two property?)

Or perhaps axis_limits? (quite descriptive i reckon)

I feel like axis_limits is used somewhere else too, but even if it isn't axis is a separate widget/visual and since it is not involved here it could be confusing if/when this visual is used in a plot alongside the AxisVisuals. I'd be open to the two separate properties if you like that. Matching matplotlib wouldn't be the worst and the two properties make it more obvious what the order of the elements are.

Bounds...extents...limits...something else? Not that my word should be final but now I'm starting to lean towards grid_bounds if we're going with a single property.

I kinda feel like the most ergonomic would be to have independent kwargs and properties (xmin, xmax, ymin, ymax, or something like that), but also xlimits and ylimits makes sense. We can always add a convenience property for the whole grid_bounds.

Let's go with grid_bounds. You can also do the split into individual limits and then wrapper, if you feel like it, but not required.

djhoese · 2023-06-27T18:54:26Z

vispy/visuals/gridlines.py

@@ -75,7 +96,9 @@ class GridLinesVisual(ImageVisual):
        channel modified.
    """

-    def __init__(self, scale=(1, 1), color='w'):
+    def __init__(self, scale=(1, 1), color='w',
+                 bounds=(-np.inf, np.inf, -np.inf, np.inf),


This needs to be added to the docstring and the element order described (xmin, ymin, xmax, ymax)?

I would also find it more intuitive to allow None as a bound as convenience, and just convert to the appropriate inf in the setter.

👍 on that idea.

Sounds good, do you want to add to this PR for that functionality?

Yes. The -inf functionality should be possible/supported, but it should probably default to 4 None and have those converted to inf as a convenience. You could even support a single =None and convert that to (-np.inf, np.inf, -np.inf, np.inf).

Also this should be added. I suggest something like this:

@bounds.setter def bounds(self, value): if value is None: value = (None,) * 4 bounds = [] for i, v in enumerate(value): if v is None: if i % 2: v = -np.inf else: v = np.inf bounds.append(v) self.shared_program['u_gridlines_bounds'] = value self._bounds = bounds self.update()

And change the init to have bounds=None

brisvag

Nice work! I left a few comments, but generally very useful and to the point :)

brisvag · 2023-06-29T12:35:39Z

vispy/visuals/gridlines.py

+    if (px_pos.z > 1 || px_pos.z < -1)
+        discard;


Could you add a comment to this? Not immediately obvious to me why it's needed.

The grid should be planar, otherwise, it will induce artifacts

Unless I'm missing something the px_pos will be the pixel position on the Canvas/screen. z should never be anything except 0, right? Or is it possible for z to not be 0 if some other transforms shift the z level of the Visual...even so that wouldn't necessarily mean that the grid isn't planar. Sorry if I'm missing something.

as mentioned, px_pos is the document (=canvas) position of the pixel, so this check is incorrect, as it's checking the depth level of the fragment. I suspect it's only working because z is always between 0 and 1 or somethign like that, so it never does anything. Does removing this check cause issues on your side @soraxas ?

I suspect what you wanted to do is rather check if the local_pos is out of plane? But even then, I doubt it ever happens, since everything is computed and never passed by the user.

brisvag · 2023-06-29T12:36:32Z

vispy/visuals/gridlines.py

+        if (all(greaterThan(local_pos.xy, u_gridlines_bounds.xz - u_border_width)) &&
+            all(lessThan(local_pos.xy, u_gridlines_bounds.yw + u_border_width))) {


If I understand correctly, this adds a border outside of the grid. I feel like maybe it's more exptected to be "on top of the outer line", with half the thickness inside and half outside... Not a big deal, just pointing out in case it's preferred.

If it's half-half, there will be strange corner cases.

For example, say, the grid is 1x1 but the thickness is 5. Then, the whole "grid" would be the border.

I think it is pretty standard/common for it to be half-half. For example, I just made a rectangle on a Google Drawing (google drive) and increasing the border size keeps it on the line of the rectangle, but it "grows" toward the inside and the outside (half on the inside, half on the outside).

I would say this has to change to half/half in order to merge. This is the standard behaviour of other visuals like markers, and the expected behaviour in most vis software. I think uglyness caused by edge cases should be solved by having more sane inputs.

brisvag · 2023-06-29T12:39:47Z

vispy/visuals/gridlines.py

@@ -75,7 +96,9 @@ class GridLinesVisual(ImageVisual):
        channel modified.
    """

-    def __init__(self, scale=(1, 1), color='w'):
+    def __init__(self, scale=(1, 1), color='w',
+                 bounds=(-np.inf, np.inf, -np.inf, np.inf),


I would also find it more intuitive to allow None as a bound as convenience, and just convert to the appropriate inf in the setter.

brisvag · 2024-05-28T09:06:42Z

@soraxas are you still interested in pushing this along? Otherwise I might pick it up at some point :)

soraxas · 2024-05-28T12:16:23Z

Hey @brisvag, yea function wise, I've been using this without any issue from my fork. As to this PR, there was just some stall on agreeing the interface I think. Any things else that you reckon need to be done?

brisvag

I think to merge this, we should address the remaining points, which I answered to again to clarify.

brisvag · 2024-05-28T14:44:14Z

vispy/visuals/gridlines.py

+    if (px_pos.z > 1 || px_pos.z < -1)
+        discard;


as mentioned, px_pos is the document (=canvas) position of the pixel, so this check is incorrect, as it's checking the depth level of the fragment. I suspect it's only working because z is always between 0 and 1 or somethign like that, so it never does anything. Does removing this check cause issues on your side @soraxas ?

I suspect what you wanted to do is rather check if the local_pos is out of plane? But even then, I doubt it ever happens, since everything is computed and never passed by the user.

brisvag · 2024-05-28T14:45:53Z

vispy/visuals/gridlines.py

+        if (all(greaterThan(local_pos.xy, u_gridlines_bounds.xz - u_border_width)) &&
+            all(lessThan(local_pos.xy, u_gridlines_bounds.yw + u_border_width))) {


I would say this has to change to half/half in order to merge. This is the standard behaviour of other visuals like markers, and the expected behaviour in most vis software. I think uglyness caused by edge cases should be solved by having more sane inputs.

brisvag · 2024-05-28T15:00:21Z

vispy/visuals/gridlines.py

@@ -75,7 +96,9 @@ class GridLinesVisual(ImageVisual):
        channel modified.
    """

-    def __init__(self, scale=(1, 1), color='w'):
+    def __init__(self, scale=(1, 1), color='w',
+                 bounds=(-np.inf, np.inf, -np.inf, np.inf),


Also this should be added. I suggest something like this:

@bounds.setter def bounds(self, value): if value is None: value = (None,) * 4 bounds = [] for i, v in enumerate(value): if v is None: if i % 2: v = -np.inf else: v = np.inf bounds.append(v) self.shared_program['u_gridlines_bounds'] = value self._bounds = bounds self.update()

And change the init to have bounds=None

brisvag · 2024-05-28T15:03:36Z

vispy/visuals/gridlines.py

@@ -86,6 +109,30 @@ def __init__(self, scale=(1, 1), color='w'):
        self.shared_program.frag['get_data'] = self._grid_color_fn
        cfun = Function('vec4 null(vec4 x) { return x; }')
        self.shared_program.frag['color_transform'] = cfun
+        self.unfreeze()
+        self.bounds = bounds


Let's go with grid_bounds. You can also do the split into individual limits and then wrapper, if you feel like it, but not required.

implements limiting size of gridlines

bc49abc

Signed-off-by: Tin Lai <oscar@tinyiu.com>

djhoese requested changes Jun 24, 2023

View reviewed changes

vispy/visuals/gridlines.py Outdated Show resolved Hide resolved

gridlines default to infinite grid and fix typo

b3bec6b

Signed-off-by: Tin Lai <oscar@tinyiu.com>

djhoese reviewed Jun 25, 2023

View reviewed changes

djhoese reviewed Jun 27, 2023

View reviewed changes

brisvag reviewed Jun 29, 2023

View reviewed changes

djhoese added type: enhancement component: visuals labels Jul 7, 2023

brisvag requested changes May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements limiting size for gridlines #2504

Implements limiting size for gridlines #2504

soraxas commented Jun 24, 2023

djhoese left a comment

soraxas commented Jun 25, 2023

djhoese Jun 25, 2023

soraxas Jun 26, 2023

soraxas Jun 26, 2023

djhoese Jun 26, 2023

soraxas Jun 26, 2023

djhoese Jun 27, 2023

brisvag Jun 29, 2023

brisvag May 28, 2024

djhoese Jun 27, 2023

brisvag Jun 29, 2023

djhoese Jun 29, 2023

soraxas Jul 7, 2023

djhoese Jul 7, 2023

brisvag May 28, 2024

brisvag left a comment

brisvag Jun 29, 2023

soraxas Jul 7, 2023

djhoese Jul 7, 2023

brisvag May 28, 2024

brisvag Jun 29, 2023

soraxas Jul 7, 2023 •

edited

djhoese Jul 7, 2023

brisvag May 28, 2024

brisvag Jun 29, 2023

brisvag commented May 28, 2024

soraxas commented May 28, 2024

brisvag left a comment

brisvag May 28, 2024

brisvag May 28, 2024

brisvag May 28, 2024

brisvag May 28, 2024

	def bounds(self, axis, view=None):
	"""Get the bounds of the Visual

	Parameters
	----------
	axis : int
	The axis.
	view : instance of VisualView
	The view to use.
	"""
	if view is None:
	view = self
	if axis not in self._vshare.bounds:
	self._vshare.bounds[axis] = self._compute_bounds(axis, view)
	return self._vshare.bounds[axis]

		if (all(greaterThan(local_pos.xy, u_gridlines_bounds.xz - u_border_width)) &&
		all(lessThan(local_pos.xy, u_gridlines_bounds.yw + u_border_width))) {

Implements limiting size for gridlines #2504

Are you sure you want to change the base?

Implements limiting size for gridlines #2504

Conversation

soraxas commented Jun 24, 2023

djhoese left a comment

Choose a reason for hiding this comment

soraxas commented Jun 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soraxas Jul 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brisvag commented May 28, 2024

soraxas commented May 28, 2024

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soraxas Jul 7, 2023 •

edited