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

WIP commit to eliminate the ._vertices arrays from images. #365

Draft
wants to merge 13 commits into
base: horizon
Choose a base branch
from

Conversation

blerner
Copy link
Member

@blerner blerner commented Dec 27, 2020

This PR changes the image implementation in a few ways:

  • Because .vertices are going away whenever possible, we no longer have to render images solely in Quadrant I
  • Introduces a BoundingBox object to track bounding boxes of images
  • Computing the bounding box of an image can be done more precisely (especially for ellipses), by passing the current transformation down the image tree, and computing the bounding box for the particular transformation as needed. These bounding boxes are memoized for the identity transformation, so that we know what the bounding box is for a standalone image
  • Rendering images to canvases shifts slightly, to translate the origin of the canvas to the center, and then again to make that match the actual middle of the image. This transformation eliminates the dependency on Quadrant I

Not all tests pass yet, some pinholes are still wrong, and possibly not all image primitives work yet. I think this should improve performance, but it's possible the additional bounding-box computations slow things down.

… changes the image implementation in a few ways:

- Because .vertices are going away whenever possible, we no longer have to render images solely in Quadrant I
- Introduces a BoundingBox object to track bounding boxes of images
- Computing the bounding box of an image can be done more precisely (especially for ellipses), by passing the current transformation *down* the image tree, and computing the bounding box for the particular transformation as needed.  These bounding boxes are memoized for the identity transformation, so that we know what the bounding box is for a standalone image
- Rendering images to canvases shifts slightly, to translate the origin of the canvas to the center, and then again to make that match the actual middle of the image.  This transformation eliminates the dependency on Quadrant I

Not all tests pass yet, some pinholes are still wrong, and possibly not all image primitives work yet.  I *think* this should improve performance, but it's possible the additional bounding-box computations slow things down.
@jpolitz jpolitz changed the title WIP commit to eliminate the .vertices arrays from images. WIP commit to eliminate the ._vertices arrays from images. Dec 28, 2020
@jpolitz
Copy link
Member

jpolitz commented Dec 28, 2020

Can you say a little more about what ._vertices were for, and what work is involved in removing them?

It's not clear to me which bullets in the PR comment are necessary in order to remove _vertices and which are new ideas/improvements.

This PR would be easiest for me to review if the answer was “all the changes are necessary in order to remove _vertices” so I don't have to consider new features/optimizations at the same time.

@blerner
Copy link
Member Author

blerner commented Dec 28, 2020

The "quadrant I dependency" is an independent thing, and I'm reconsidering it already since it currently slightly breaks how place-pinhole works, according to its own documentation. That's fixable independent of this, though.

The main use for the .vertices array was that polygonal images could store their vertices and use that as a fast path for image equality. The problem was that creating those arrays got expensive, quickly. Then there's a secondary ._vertices array, for when some images needed to store vertices without triggering equality checking based on .vertices: consider the difference between an n-agon and an n-pointed star. Then over time, some additional uses of the vertices arrays seemed to arise, but they don't seem particularly needed.

I've eliminated the copying of .vertices all over the place, especially in Scale/Rotate/OverlayImages, but that introduces another potential problem: computing bounding boxes. Essentially, there are two ways to compute bounding boxes: eagerly or lazily. The eager approach computes the bounding box for an image when it's created, and assumes that it's fully filling out its rectangular area. But when that image gets composed into larger images, the bounding box won't be tight. The current implementation has this problem:

include image

c1 = circle(30, "solid", "red")
c2 = circle(30, "solid", "blue")
o1 = overlay-xy(c1, 20, 20, c2)
[list:
  frame(o1),
  frame(rotate(-45, o1)),
  frame(rotate(-45, overlay-xy(frame(c1), 20, 20, frame(c2))))]

produces
image
The bounding boxes around the ellipses in the second image aren't tight because they're not polygonal images and so the computation assumes they fill their entire rectangle, as shown by the frames in the third image. However, for polygonal images, because of the .vertices array, the image library currently does something different:

t1 = triangle(30, "solid", "red")
t2 = triangle(30, "solid", "blue")
o2 = overlay-xy(t1, 20, 20, t2)
[list:
  frame(o2),
  frame(rotate(-45, o2)),
  frame(rotate(-45, overlay-xy(frame(t1), 20, 20, frame(t2)))),
  frame(rotate(-45, frame(o2)))]

produces
image
The second image is tight around the triangles, while the third image is tight around their individual frames and the fourth is tight around the combined frame.

The only way I know of to compute the correct bounding box for an ellipse is the lazy way: by traversing the image tree, passing down the current transformation, and then doing some math on that transformation matrix to compute the tight bounds of the ellipse.

The same circle code above, in my branch, produces
image
(which matches what DrRacket produces, but not WeScheme)

Unfortunately, my current branch is slower than I expected: I think these extra transformation matrices are outweighing the benefits of getting rid of the vertices arrays. More work is needed to figure out what's going on...

Lastly, there are two image tests that are failing against our regression suite: they appear to be ~1px different in size than before, which I think is due solely to rounding differences in coordinate calculations; the images visually look identical to me.

Ben Lerner added 10 commits December 28, 2020 14:27
- resetting pinhole coordinate system to top-left, per docs
- fixing overlay somewhat more, so that e.g. overlaying a triangle on a circle never overflows the circle
- adding color-frame and color-pinhole, for additional debugging.

Three tests currently fail, and arguably the reference images need fixing
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