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.

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

Sierpinski's Triangle example #813

Merged
merged 18 commits into from
Jun 18, 2021
Merged

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented May 12, 2021

This PR introduces a new example for visualizing Sierpinski's Triangle.

We currently have a kiva tutorial and maybe eventually it could be useful to turn this into a comparable enable tutorial? It will need to be cleaned up first.

Some things of note: Sierpinski's Triangle can be coded in a cleaner way (and this can be down with enabl/kiva) where you have a draw triangle method and a midpoint method where basically you draw a triangle and then another one using the midpoints of the original as vertices for the next, and then use the midpoints of each of the 3 resulting triangles to draw 3 more and so on.

I was trying to make use of draw_marker_at_points here (initially) and so was explicitly calculating the marker locations relative to a previous marker location. I ultimately could not use draw_marker_at_points because I realized the kiva INVERTED_TRIANGLE_MARKER was not drawing how I expected based on the _add_to_path method for the corresponding enable marker. It did not appear to be drawing equilateral triangles, and they were shifted slightly. Because it was more obvious to see exactly how the path was being drawn via the _add_to_path method I simply used draw_path_at_points (it is more generally available IIRC across backends anyway).

As a part of this motivation to use draw_path_at_points in my recursive method to find where to draw triangles, rather than drawing them in that method (which is what I had originally done), I instead stored the locations / sizes in a point_contexts dictionary (naming things is hard and the current naming in this PR is very ugly). The number of triangles to draw increases by a power of 3 every iteration, so this way we can make self.iterations calls to draw_path_at_points rather than once call for every 3 drawn triangles.

There are still improvements I want to make, the biggest one being adding zoom which I am not exactly sure how to do currently...

Copy link
Member

@jwiggins jwiggins left a comment

Choose a reason for hiding this comment

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

Thanks for the example. Sierpinski's triangle is one of my favorite fractals.

I feel like this could be done without a marker object, but I'm still sore about #390 and #716. I think you've basically got the math already for drawing a triangle of arbitrary scale given a center point. You can still minimize draw calls by recursively building a path object and then drawing it in one call.

(don't forget to flake this before merging)

enable/examples/demo/enable/sierpinski.py Outdated Show resolved Hide resolved
enable/examples/demo/enable/sierpinski.py Outdated Show resolved Hide resolved

class SierpinskiTriangle(Component):

base_width = Int(500)
Copy link
Member

Choose a reason for hiding this comment

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

Did you have a plan to update this when the size of the component changed?

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 had originally been thinking to do that but I might hold off and do it as a later improvement in a future PR

enable/examples/demo/enable/sierpinski.py Outdated Show resolved Hide resolved
enable/examples/demo/enable/sierpinski.py Outdated Show resolved Hide resolved
enable/examples/demo/enable/sierpinski.py Outdated Show resolved Hide resolved
@aaronayres35
Copy link
Contributor Author

@jwiggins thank you for the review!

@aaronayres35 aaronayres35 requested a review from rahulporuri May 19, 2021 13:59
@aaronayres35
Copy link
Contributor Author

aaronayres35 commented Jun 8, 2021

You can still minimize draw calls by recursively building a path object and then drawing it in one call.

I'm not exactly sure how to do this using the marker, because the markers _add_to_path method seems to assume it is drawing at (0,0). I tried to, rather than calling draw_path_at_points, create a singular path, move to the point for each triangle to be drawn and call marker.add_to_path on that path, then call gc.add_path on that path.
However when doing this all the triangles draw at the origin.

e.g:

Screen Shot 2021-06-07 at 7 31 57 PM Screen Shot 2021-06-07 at 7 31 46 PM

It seems lines works with absolute locations.

If I were to try to continue to follow the above suggestion, I could instead just write my own add_to_path function which simply adds a triangle to the path relative to a certain point. However at that point, the marker becomes unneeded. Or, could use translate_ctm but then the calculation of points for the triangles may need to change.
At that point, I might as well rewrite it entirely to not use markers, and not work with "centers" of the triangles which simplifies the math. I will work on this but given it will involve a large rework of the example, I might just open a separate PR implementing that approach.

@jwiggins
Copy link
Member

jwiggins commented Jun 8, 2021

You can still minimize draw calls by recursively building a path object and then drawing it in one call.

I'm not exactly sure how to do this using the marker, because the markers _add_to_path method seems to assume it is drawing at (0,0).

Don't use markers or GC transforms. Just create a path object using recursive calls and then draw it with a single draw call. That means you're doing "all the math" yourself, but it's not that bad because the recursion makes it straightforward. You're using the call stack rather than the transformation stack.

Comment on lines +158 to +164
self.viewport = Viewport(
component=canvas, enable_zoom=True, stay_inside=True
)
self.viewport.zoom_tool.min_zoom = 1.0
self.viewport.tools.append(
ViewportPanTool(self.viewport, drag_button="right")
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Viewport is very strange... It has a zoom_tool trait so zoom is handled differently from other tools, eg ViewportPanTool. Additionally, with this line:

if "zoom_tool" not in traits:
self.zoom_tool = ViewportZoomTool(self)

we create a ViewportZoomTool instance by default if enable_zoom is True, passing in self (the viewport) as the component. That is all fine and good, but I was struggling to see how I could pass in a zoom_tool other than this... If I want to pass in my own zoom tool when instantiation a viewport, eg:

my_zoom_tool = ViewportZoomTool(...)
my_viewport = Viewport(..., zoom_tool=my_zoom_tool)

I was running into a chicken and egg scenario as I needed the my_viewport instance to pass in as the component for the ViewportZoomTool, but it isn't defined when I define my_zoom_tool. If I try to switch the order:

my_viewport = Viewport(..., zoom_tool=my_zoom_tool)
my_zoom_tool = ViewportZoomTool(...)

Then the above line gets run so I end up using the default zoom tool, not my_zoom_tool. Thus I resorted to just setting the trait I wanted to change on the default zoom tool that got created, rather than trying to instantiate my own.
This all works as I want it to. However, I do not understand when a user would want / be able to set the zoom_tool trait. Maybe if the ViewportZoomTool was working with a second Viewport or something?
However, as far as I understand these are always with a Canvas (that very well may not actually be true, but there isn't documentation on this / there aren't many examples) , and Canvas has the comment:

enable/enable/canvas.py

Lines 34 to 35 in 7f3231d

viewport that is looking at it. (TODO: add support for multiple
viewports.)

so that doesn't seem possible either.

Additionally, ViewportZoomTool has traits max_zoom / min_zoom, but its base class BaseZoomTool has traits max_zoom_in_factor and min_zoom_in_factor which appear to have the same purpose.

There are still many things about Viewport and ViewportZoomTool I do not fully understand / I was unable to get to work how I expected. e.g. the {horizontal/vertical}_anchor traits, stay_inside / view_bounds.

In any case, this example effectively works as I want it to. Perhaps I should open some issues about these things / an issue about adding documentation on how to use zoom in enable generally. I imagine enable zoom is a pretty niche thing as chaco handles zoom on its own, but even so.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah.. I'm not entirely sure what use case this stuff was initially built for but it likely doesn't exist anymore. The quirks are the interface at this point, unfortunately.

@aaronayres35 aaronayres35 changed the title [WIP] Sierpinski's Triangle example Sierpinski's Triangle example Jun 17, 2021
@aaronayres35
Copy link
Contributor Author

I've removed the WIP label from this PR. I think there are certainly still improvements that can be made, but I think I will leave them for follow up PRs now.

@aaronayres35 aaronayres35 requested a review from jwiggins June 18, 2021 14:35
@aaronayres35
Copy link
Contributor Author

@jwiggins no rush at all, but if you do have a chance to take another look at this PR I would greatly appreciate your feedback!

Copy link
Member

@jwiggins jwiggins left a comment

Choose a reason for hiding this comment

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

This is good enough as is to merge now. If you're looking for follow-up work to do, I would consider caching the path which is generated so that you don't have to do the expensive recursive call for every draw. Performance gets really sluggish if you zoom in and crank the iterations up.

enable/examples/demo/enable/sierpinski_zoom.py Outdated Show resolved Hide resolved
enable/examples/demo/enable/sierpinski_zoom.py Outdated Show resolved Hide resolved
enable/examples/demo/enable/sierpinski_zoom.py Outdated Show resolved Hide resolved
Comment on lines +158 to +164
self.viewport = Viewport(
component=canvas, enable_zoom=True, stay_inside=True
)
self.viewport.zoom_tool.min_zoom = 1.0
self.viewport.tools.append(
ViewportPanTool(self.viewport, drag_button="right")
)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah.. I'm not entirely sure what use case this stuff was initially built for but it likely doesn't exist anymore. The quirks are the interface at this point, unfortunately.

@aaronayres35
Copy link
Contributor Author

@jwiggins thank you for the reviews!

@aaronayres35 aaronayres35 merged commit 1b8c3fa into master Jun 18, 2021
@aaronayres35 aaronayres35 deleted the sierpinski-triangle-example branch June 18, 2021 20:37
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