Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove all definitions / uses of _draw_component #743

Merged
merged 4 commits into from
Jun 8, 2021
Merged

Remove all definitions / uses of _draw_component #743

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a582e4
remove all definitions of _draw_component
aaronayres35 May 18, 2021
357efce
flake8
aaronayres35 May 18, 2021
22b81ae
component is not an argument for _draw_overlay
aaronayres35 Jun 1, 2021
91ffee7
Merge branch 'master' into _draw_component-cleanup
aaronayres35 Jun 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 27 additions & 15 deletions chaco/axis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,31 +244,43 @@ def overlay(self, component, gc, view_bounds=None, mode="normal"):
"""
if not self.visible:
return
self._draw_component(gc, view_bounds, mode, component)

if not self._cache_valid:
if component is not None:
self._calculate_geometry_overlay(component)
else:
self._calculate_geometry()
self._compute_tick_positions(gc, component)
self._compute_labels(gc)

with gc:
# slight optimization: if we set the font correctly on the
# base gc before handing it in to our title and tick labels,
# their set_font() won't have to do any work.
gc.set_font(self.tick_label_font)

if self.axis_line_visible:
self._draw_axis_line(
gc, self._origin_point, self._end_axis_point
)
if self.title:
self._draw_title(gc)

self._draw_ticks(gc)
self._draw_labels(gc)

self._cache_valid = True

def _draw_overlay(self, gc, view_bounds=None, mode="normal"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_draw_overlay and overlay are now currently the exact same method. Perhaps one should just call the other to avoid the duplication. I think it would make more sense to have overlay call _draw_overlay.
AFAIK we need to keep both methods, but this should be investigated more closely.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense to have overlay call _draw_overlay.

sounds good to me.

AFAIK we need to keep both methods, but this should be investigated more closely.

I'm not a 100% sure why both methods need to exist. I know that _draw_overlay is needed by the new (and currently used) way of drawing the various layers in a chaco plot. Not sure what overlay is used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this PR again I realize that overlay and _draw_overlay do very slightly different things. I think it is going to be worth the deeper investigation into what both are uesd for / if we can refine things down so only 1 is necessary at all.
If they are in fact both needed it is possible they will need to exist stand alone with very similar (largely copied) code, but some minor discrepancies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Digging into enable:
overlay is a method defined on AbstractOverlay, and it takes other_component as an argument. Effectively the overlay draws itself on top of another component.

_draw_overlay originates from Component which keeps track of a list of AbstractOverlays, and when _draw_overlay is called, it iterates over each of those overlays and calls their respective overlay methods, passing in itself as the other_component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I think what makes sense is that classes which are decedents from AbstractOverlay should only need to define overlay not _draw_overlay.

It is a bit strange because AbstractOverlay subclasses Component so it does technically have a _draw_overlay method. However, for Component _draw_overlay gets called via the _draw method. But _draw is overridden by AbstractOverlay and simply calls overlay if self.component is not None.

So, in summary, I believe we can delete all the definitions of _draw_overlay on subclasses of AbstractOverlay since they are actually unused. overlay is the method that actually does the work. Since the current classes which do override _draw_overlay do not also override _draw, nothing ever actually calls _draw_overlay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking closer, some differences between enable.AbstractOverlay and chaco.AbstractOverlay include:

  1. enable defines _do_layout (default is just pass), and do_layout methods, which chaco does not. do_layout (per the docstring) differs from the Component.do_layout method in that it allows passing an optional component arg.
  2. in the _draw method, chaco calls super if self.component is None. enable simply returns.

Number 2 is what makes me think maybe there are scenarios in which _draw_component gets called / is needed if we try to draw an Overlay for which component is None. ie a stand alone overlay (which I will have to look for use cases / examples of ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For instance, the _draw_overlay use on Tooltip and its subclass DataLabel is needed, since they override draw and call PlotComponent._draw directly.

I do not see any cases in which PlotAxis does not have a component set (every time it is instantiated in the chaco codebase a component is passed in), however theoretically one could try drawing an Axis standalone (although i can't imagine why) in which case not having _draw_overlay will cause a break...

I am of two minds if it should be removed or not for the cases of PlotAxis and Grid here. If it is to stay, because the two methods do slightly different things, I believe they will both need to stay as is with some duplication (ie overlay can't just call _draw_overlay).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rahulporuri I apologize in advance that this is an overly wordy brain dump, but what are your thoughts on this? I hope my findings make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't fully digest these findings at the moment - so my instinct is to simply defer this question to a later time. Can you write up these findings in an issue? I apologize for the delay in responding to your comments - it'll probably involve additional effort to recollect everything and put together a meaningful PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I will open an issue

"""Draws the overlay layer of a component.

Overrides PlotComponent.
"""
self._draw_component(gc, view_bounds, mode)

def _draw_component(
rahulporuri marked this conversation as resolved.
Show resolved Hide resolved
self, gc, view_bounds=None, mode="normal", component=None
):
"""Draws the component.

This method is preserved for backwards compatibility. Overrides
PlotComponent.
"""
if not self.visible:
return

if not self._cache_valid:
if component is not None:
self._calculate_geometry_overlay(component)
else:
self._calculate_geometry()
self._calculate_geometry()
self._compute_tick_positions(gc, component)
self._compute_labels(gc)

Expand Down
6 changes: 0 additions & 6 deletions chaco/base_xy_plot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,12 +474,6 @@ def get_screen_points(self):

def _draw_plot(self, gc, view_bounds=None, mode="normal"):
"""Draws the 'plot' layer."""
self._draw_component(gc, view_bounds, mode)

def _draw_component(self, gc, view_bounds=None, mode="normal"):
# This method should be folded into self._draw_plot(), but is here for
# backwards compatibilty with non-draw-order stuff.
Comment on lines -490 to -491
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment effectively sums up the aim of this PR


pts = self.get_screen_points()
self._render(gc, pts)

Expand Down
2 changes: 1 addition & 1 deletion chaco/colormapped_scatterplot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def _draw_plot(self, gc, view_bounds=None, mode="normal"):
# Take into account fill_alpha even if we are rendering with only two values
old_color = self.color
self.color = tuple(self.fill_alpha * array(self.color_))
super()._draw_component(gc, view_bounds, mode)
super()._draw_plot(gc, view_bounds, mode)
self.color = old_color
else:
colors = self._cached_data_pts[:, 2]
Expand Down
58 changes: 44 additions & 14 deletions chaco/grid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,31 +333,59 @@ def _draw_overlay(self, gc, view_bounds=None, mode="normal"):

Overrides PlotComponent.
"""
self._draw_component(gc, view_bounds, mode)
# What we're really trying to do with a grid is plot contour lines in
# the space of the plot. In a rectangular plot, these will always be
# straight lines.
if not self.visible:
return

def overlay(self, other_component, gc, view_bounds=None, mode="normal"):
"""Draws this component overlaid on another component.
if not self._cache_valid:
self._compute_ticks()

Overrides AbstractOverlay.
"""
if not self.visible:
if len(self._tick_positions) == 0:
return
self._compute_ticks(other_component)
self._draw_component(gc, view_bounds, mode)
self._cache_valid = False

def _draw_component(self, gc, view_bounds=None, mode="normal"):
rahulporuri marked this conversation as resolved.
Show resolved Hide resolved
"""Draws the component.
with gc:
gc.set_line_width(self.line_weight)
gc.set_line_dash(self.line_style_)
gc.set_stroke_color(self.line_color_)
gc.set_antialias(False)

This method is preserved for backwards compatibility. Overrides
PlotComponent.
if self.component is not None:
gc.clip_to_rect(
*(self.component.position + self.component.bounds)
)
else:
gc.clip_to_rect(*(self.position + self.bounds))

gc.begin_path()
if self.orientation == "horizontal":
starts = self._tick_positions.copy()
starts[:, 0] = self._tick_extents[:, 0]
ends = self._tick_positions.copy()
ends[:, 0] = self._tick_extents[:, 1]
else:
starts = self._tick_positions.copy()
starts[:, 1] = self._tick_extents[:, 0]
ends = self._tick_positions.copy()
ends[:, 1] = self._tick_extents[:, 1]
if self.flip_axis:
starts, ends = ends, starts
gc.line_set(starts, ends)
gc.stroke_path()

def overlay(self, other_component, gc, view_bounds=None, mode="normal"):
"""Draws this component overlaid on another component.

Overrides AbstractOverlay.
"""
# What we're really trying to do with a grid is plot contour lines in
# the space of the plot. In a rectangular plot, these will always be
# straight lines.
if not self.visible:
return

self._compute_ticks(other_component)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method now only differs from _draw_overlay in this line and the self._cache_valid = False line as this is what overlay had done differently previously as well


if not self._cache_valid:
self._compute_ticks()

Expand Down Expand Up @@ -393,6 +421,8 @@ def _draw_component(self, gc, view_bounds=None, mode="normal"):
gc.line_set(starts, ends)
gc.stroke_path()

self._cache_valid = False

def _mapper_changed(self, old, new):
if old is not None:
old.observe(self.mapper_updated, "updated", remove=True)
Expand Down
38 changes: 15 additions & 23 deletions chaco/lasso_overlay.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,21 @@ def overlay(self, other_component, gc, view_bounds=None, mode="normal"):
with gc:
c = other_component
gc.clip_to_rect(c.x, c.y, c.width, c.height)
self._draw_component(gc, view_bounds, mode)
with gc:
# We may need to make map_screen more flexible in the number of
# dimensions it accepts for ths to work well.
for selection in self.lasso_selection.disjoint_selections:
points = self.component.map_screen(selection)
if len(points) == 0:
return
points = concatenate((points, points[0, newaxis]), axis=0)
gc.set_line_width(self.selection_border_width)
gc.set_line_dash(self.selection_border_dash_)
gc.set_fill_color(self.selection_fill_color_)
gc.set_stroke_color(self.selection_border_color_)
gc.set_alpha(self.selection_alpha)
gc.lines(points)
gc.draw_path()

def _updated_changed_for_lasso_selection(self):
self.component.invalidate_draw()
Expand All @@ -58,25 +72,3 @@ def _event_state_fired_for_lasso_selection(self, val):
self._draw_selection = val == "selecting"
self.component.invalidate_draw()
self.component.request_redraw()

def _draw_component(self, gc, view_bounds=None, mode="normal"):
"""Draws the component.

This method is preserved for backwards compatibility with _old_draw().
Overrides PlotComponent.
"""
with gc:
# We may need to make map_screen more flexible in the number of dimensions
# it accepts for ths to work well.
for selection in self.lasso_selection.disjoint_selections:
points = self.component.map_screen(selection)
if len(points) == 0:
return
points = concatenate((points, points[0, newaxis]), axis=0)
gc.set_line_width(self.selection_border_width)
gc.set_line_dash(self.selection_border_dash_)
gc.set_fill_color(self.selection_fill_color_)
gc.set_stroke_color(self.selection_border_color_)
gc.set_alpha(self.selection_alpha)
gc.lines(points)
gc.draw_path()
5 changes: 0 additions & 5 deletions chaco/polar_line_renderer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,6 @@ def _downsample(self):

def _draw_plot(self, *args, **kw):
"""Draws the 'plot' layer."""
# Simple compatibility with new-style rendering loop
return self._draw_component(*args, **kw)

def _draw_component(self, gc, view_bounds=None, mode="normal"):
"""Renders the component."""
self._gather_points()
self._render(gc, self._cached_data_pts)

Expand Down
4 changes: 2 additions & 2 deletions chaco/tools/regression_lasso.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ class RegressionOverlay(LassoOverlay):
),
)

def _draw_component(self, gc, view_bounds=None, mode="normal"):
super()._draw_component(gc, view_bounds, mode)
def overlay(self, other_component, gc, view_bounds=None, mode="normal"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this by running examples/demo/basic/regression.py and everything seems to work as expected

super().overlay(other_component, gc, view_bounds, mode)
selection = self.lasso_selection

if selection.fit_params is not None:
Expand Down