-
Notifications
You must be signed in to change notification settings - Fork 616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implements limiting size for gridlines #2504
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tin Lai <oscar@tinyiu.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: Tin Lai <oscar@tinyiu.com>
Thanks for pointing out the typo and yep the previous infinite grid is set to be its default behaviour |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... perhaps we can just literally name it as gridline_bounds
? (not too creative i know...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 xlim
and ylim
? (might need to split them into two property?)
Or perhaps axis_limits
? (quite descriptive i reckon)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be added to the docstring and the element order described (xmin, ymin, xmax, ymax)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on that idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, do you want to add to this PR for that functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I left a few comments, but generally very useful and to the point :)
if (px_pos.z > 1 || px_pos.z < -1) | ||
discard; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment to this? Not immediately obvious to me why it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grid should be planar, otherwise, it will induce artifacts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@soraxas are you still interested in pushing this along? Otherwise I might pick it up at some point :) |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to merge this, we should address the remaining points, which I answered to again to clarify.
if (px_pos.z > 1 || px_pos.z < -1) | ||
discard; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This closes #2311
Test:
Custom limits and boarder width
Equivalent of infinite grid