Add flip procedures

[benknoble]

I'm not actually sure if the contract pict-convertible? is right, but it's what scale had and it also uses pict-width and pict-height on a pict-convertible?, so I went with it.

(Is this one of those contracts like Butterick's sugar that automatically does the conversion? Or is pict-width exported as a wrapper? Because the docs suggest that pict-width is the struct accessor, so the implementation of scale and thus the flipping functions is actually at fault relative to its contract.)

[benknoble]

From locally-built docs of this PR.

[soegaard]

I'm not actually sure if the contract pict-convertible? is right, ...
I think it is.

If I am reading "pict.rkt" correctly, pict is not a normal struct.
The accessors convert to a pict if needed.

https://github.com/racket/pict/blob/master/pict-lib/pict/private/pict.rkt

[benknoble]

I'm not actually sure if the contract pict-convertible? is right, ...

I think it is.

If I am reading "pict.rkt" correctly, pict is not a normal struct.

The accessors convert to a pict if needed.

https://github.com/racket/pict/blob/master/pict-lib/pict/private/pict.rkt

Oh, phew. That makes sense.

[benknoble]

If there's something else needed to merge this (tests? version note?), I'll be happy to dig into it, but otherwise this is complete from my end.

[rfindler]

I'm not really complete sure how the transformation matricies work but, the specific linear algebra aside, this looks good to me.

I agree it would be nice to have a test that (equal? (flip-x (flip-x <p>)) <p>) for some interesting pict <p> would be a good addition to the test suite.

I had thought that this would be implemented with a scale of -1, actually (but that's neither here nor there).

[soegaard]

The scale with -1 trick almost works - but the bounding box ends up wrong.

See the original issue:

#29

[rfindler]

Ah, thanks.

And my comment about equal? and flipping was based on a confusion of mine, thinking about 2htdp/image not pict (where equality is an important issue). It still does seem nice that there be some tests.

Perhaps a test case that would distinguish this implementation from the "scale by negative one" approach?

[benknoble]

I'll have to take a look at how pict testing is done, but I agree it would be nice for flip-x and flip-y to be tested for compositions and against the bounding-box issue.

[benknoble]

Tests passed 100% according to raco test . in pict-test.

[rfindler]

Does (check-pict=? (flip-x (ellipse 20 30)) (ellipse 20 30)) and similarly for flip-y pass?

(I think this won't pass with the scale-based implementation.)

[benknoble]

Hmm, so far they don't seem to pass (failure reported for (flip-x oval) versus oval, which the obvious (define oval (ellipse 20 30))). Honestly, I'm not sure why this would be the case, and it's a good test case :)

[rfindler]

I was playing around a little bit and saw this:

Looking at the code, I wonder if the dx and dy arguments need to be taken into account when defining the transformations? That's just a shot in the dark, tho!

[soegaard]

You (Robby) were right, the dx and dy arguments were missing from the transformations.

In flip-x it needs to be:

        (define new-t (compose-trans*
                        (make-translate (+ x (/ w 2)) y)
                        (make-flip-x)
                        (make-translate (- (/ w 2)) 0)
                        old-t))

When applied the transformations work from bottom to top.
So here

1. The old initial transformation is applied: old-t
2. We move the center of the pict to (0,0): (make-translate (- (/ w 2)) 0)
3. We scale x with -1 flipping the image: (make-flip-x)
4. We move the center of the pict to (x,y): (make-translate (+ x (/ w 2)) y)

Similarly the transformation in flip-y needs to be:

        (define new-t (compose-trans*
                        (make-translate x (+ y (/ h 2)))
                        (make-flip-y)
                        (make-translate 0 (- (/ h 2)))
                        old-t))

With these changes Robby's example work again and the ellipse tests pass too.

(define e (ellipse 20 30))
(check-pict=? (flip-x (flip-x e)) e)
(check-pict=? (flip-y (flip-y e)) e)

[benknoble]

I'll get these added today!

[benknoble]

I made @soegaard's changes, but the test (check-pict=? (flip-x oval) oval) still fails. The following example makes me think it's not "pixel perfect," but I can't tell if that's because of unintentional asymmetry in ellipse or a bug in flip-x:

,r pict racket/gui
(show-pict (cc-superimpose (ellipse 200 300 #:border-color "red")
                           (flip-x (ellipse 200 300))))

[soegaard]

I made @soegaard's changes, but the test (check-pict=? (flip-x oval) oval) still fails.
The following example makes me think it's not "pixel perfect," but I can't tell if that's
because of unintentional asymmetry in ellipse or a bug in flip-x:

It depends on your perspective. The default setting for "smoothing" is 'aligned.
That setting can move the ellipse by half a pixel. So I think the ellipse drawn
by ellipse doesn't have exact center in (w/2, h/2).

To test this, I tried the same example, but with 'smoothed and 'unsmoothed insted of 'aligned.
It's not pixel perfect, but it's close. I wouldn't be surprised, if 'smoothed is perfect for svg images.

(require (only-in metapict smoothed unsmoothed aligned))

(show-pict
  (cc-superimpose (unsmoothed (ellipse 200 300 #:border-color "red"))
                  (unsmoothed (flip-x (ellipse 200 300)))))

(show-pict
  (cc-superimpose (smoothed (ellipse 200 300 #:border-color "red"))
                  (smoothed (flip-x (ellipse 200 300)))))

My perspective is that this sort of discrepancy is expected when using 'aligned.
However, if there is a way to calculate the offset between aligned center and the unaligned center,
we can add the offset like this:

(make-translate (+ x x-offset)   (+ y y-offset  (/ h 2)))

[rfindler]

I don't have a strong opinion on whether or not this should work for 'aligned but it would be a nice test case to work for 'smoothed mode, as long as some strange floating point rounding error doesn't get us in trouble. It may also help to draw a filled ellipse with a transparent pen, eg (filled-ellipse 20 30 #:draw-border? #f).

Or, failing that, maybe a test case with a rectangle (but drawn inside a pict with a larger bounding box), perhaps something like (cc-superimpose (colorize (filled-rectangle 40 40 #:draw-border? #f) "green") (filled-rectangle 20 10 #:draw-border? #f))?

[soegaard]

Turns out I made a mistake earlier. I thought of the (dx,dy) as the coordinates of the pict center,
but it is in fact the top-left corner.

(define (flip-x p)
  (define w (pict-width p))
  (define h (pict-height p))
  (dc (λ (dc x y)
        ; ( x, y) is the top-left corner
        ; (cx,cy) is the center of the pict
        (define cx (+ x (/ w 2)))
        (define cy (+ y (/ h 2)))
        (define old-t (send dc get-initial-matrix))
        (define new-t (compose-trans*
                        (make-translate cx cy)
                        (make-flip-x)
                        (make-translate (- cx) (- cy))
                        old-t))
        (send dc set-initial-matrix new-t)
        (draw-pict p dc x y)
        (send dc set-initial-matrix old-t))
      w h))

This test revealed the problem:

(cc-superimpose
 (colorize (filled-rectangle 40 40 #:draw-border? #f) "green")
 (flip-x (filled-rectangle 20 10 #:draw-border? #f)))

Sadly this example still isn't pixel perfect.

 (show-pict
    (cc-superimpose (unsmoothed (colorize (ellipse 201 301) "red"))
                   (unsmoothed (flip-x   (ellipse 201 301)))))

[soegaard]

FWIW the svg version looks correct when examined in Inkscape.

(require (only-in metapict save-pict))

(save-pict "ellipse.svg"
           (cc-superimpose (colorize (ellipse 201 301) "red")
                           (flip-x   (ellipse 201 301)))
           'svg)

[benknoble]

I've incorporated the changes for centering/corners, but as pointed out it is not enough for pixel-perfect transforms. I can try to adjust the test to use smoothing for those picts, or we might need to find a different way to express "equivalence with some tolerance."

[rfindler]

I cherry-picked the current version of this pull request into my copy of the pict repo and then ran this program:

#lang racket/gui
(require pict)
(show-pict
 (vl-append
  (cc-superimpose
   (colorize (filled-rectangle 40 40 #:draw-border? #f) "green")
   (flip-x (filled-ellipse 20 30 #:draw-border? #f)))
  (cc-superimpose
   (colorize (filled-rectangle 40 40 #:draw-border? #f) "green")
   (filled-ellipse 20 30 #:draw-border? #f))))

and this is the output I see:

Did I do something wrong?

[benknoble]

No, I haven't pushed the latest changes while we were… debating? …the ellipse issue. I'll push it by Monday; hopefully that fixes what you're seeing.

This is almost certainly the miscalculation about the meaning of dcs arguments being top-left corner.

[rfindler]

Oh, okay! Sorry! Thanks, Robby

…

On Sat, Mar 18, 2023 at 10:16 PM D. Ben Knoble ***@***.***> wrote: No, I haven't pushed the latest changes while we were… debating? …the ellipse issue. I'll push it by Monday; hopefully that fixes what you're seeing. — Reply to this email directly, view it on GitHub <#78 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBNMEQJGEXVSBMW4HBFZ3W4Z3AHANCNFSM6AAAAAAVZOMY3U> . You are receiving this because you commented.Message ID: ***@***.***>

[benknoble]

Good thing you weren't sitting around waiting on me @rfindler 😅 just re-pushed changes with centering fix.

[rfindler]

Thanks @benknoble . Looks to me like the images are the same now. I just ran this on macos not linux, tho. But this program produces #t for me. Do you see something different?

#lang racket/gui
(require pict)
(define p1
  (cc-superimpose
   (colorize (filled-rectangle 40 40 #:draw-border? #f) "green")
   (flip-x (filled-ellipse 20 30 #:draw-border? #f))))
(define p2
  (cc-superimpose
   (colorize (filled-rectangle 40 40 #:draw-border? #f) "green")
   (filled-ellipse 20 30 #:draw-border? #f)))

(define (->bitmap p)
  (define b (pict->bitmap p))
  (define w (send b get-width))
  (define h (send b get-height))

  (define its (make-bytes
               (*
                w h
                4)
               255))
  (send b get-argb-pixels
        0 0
        (send b get-width)
        (send b get-height)
        its)
  (define mask (send b get-loaded-mask))
  (when mask
    (send b get-argb-pixels 0 0 w h its #t))
  its)

(equal? (->bitmap p1)
        (->bitmap p2))

[benknoble]

@rfindler your program does indeed print #t, but the test

pict/pict-test/tests/pict/main.rkt

Lines 754 to 756 in dc72758

	(define oval (ellipse 20 30))
	(check-pict=? (flip-x oval) oval "flipping oval in X")
	(check-pict=? (flip-y oval) oval "flipping oval in Y")

fails even with raco test -y.

If I make the change

-  (define oval (ellipse 20 30))
+  (define oval (colorize (filled-ellipse 20 30 #:draw-border? #f) "green"))

the tests pass. Similarly without colorize, but removing #:draw-border? fails.

So, since the borders are what cause problems, possibly due to the reasons @soegaard explained, what do you want to do?

[rfindler]

Well, I can't tell if it is a rounding error, something that's a bug in this transformation, or a bug in how transformations work in general that has nothing to do with flipping, so I guess the best thing to do is to put in the test cases that we know work properly (some with one flip of an ellipse in both x and y but without any pen drawing) and then merge this PR.

FWIW, I am not sure that this problem is specific to 'aligned mode; when I change the example I sent above to pass 'smoothed as the second argument to pict->bitmap, I see the same result; there are just a few pixels different. That said, it might be the case that in 'smoothed mode the error is rounding error due to floats and in 'aligned mode flipping the border doesn't actually work, however. If I total up the absolute value of the difference in pixel values and divide by 256 times 3 times the number of pixels, I get 1.46484375e-5 in smoothed mode and about 0.01 in aligned mode which would be consistent with that, I suppose. Still, however, it doesn't seem right to base a test case on that.

[soegaard]

Just found the following bug report.

racket/draw#26

If I understand correctly, the border of an ellipse is alligned, but the filled area isn't.
Does this explain what we are seeing?

[rfindler]

That would seem to support the "it is a rounding error in smoothed mode and actually a different image in aligned mode" theory, I guess?

[benknoble]

I confess I haven't followed most of the rounding/border/draw issues, but I pushed the test that works.

[benknoble]

Probably not worth including here (or even making a PR for…), but

pict/pict-test/tests/pict/main.rkt

Lines 26 to 27 in 2541f12

	(send b get-width)
	(send b get-height)

use (send b …) instead of the saved w/h.

[rfindler]

I don't see any more improvements to make; seems ready to go to me.

[rfindler]

@soegaard any thing that should be done before merging that you see?

[soegaard]

No, looks good to me.

[rfindler]

Thanks!