sicp-lang/sicp

vector-ref: contract violation for transforming images of certain sizes

pmaddams opened this issue · 7 comments

pmaddams commented

Hi Jens,

The following code works as expected until the last line.

#lang sicp

(#%require sicp-pict)

(paint einstein)
(paint (flip-horiz einstein))
(paint (flip-vert einstein))

(define wave (load-painter "wave.gif"))

(paint wave)
(paint (flip-horiz wave))
(paint (flip-vert wave))

At this point it throws the error:

vector-ref: contract violation
expected: vector?
given: 0
argument position: 1st
other arguments...:

The image in question is this one: wave.

Only some transformations cause errors. These include flip-vert, rotate90, and rotate180. Others such as flip-horiz and rotate270 did not fail.

But if you increase the height of the image by one pixel, that is, change the size from 182x182 to 182x183, all of these transformations work as expected.

Based on this information, I tried several other things.

  • Other 182x182 images do NOT work.
  • Increasing the width by one pixel, i.e. 183x182, does NOT work.
  • Either slightly smaller or slightly larger images (181x181, 183x183) as well as other dimensions such as 128x128 or 400x400, seem to work.
  • The image format, i.e. GIF vs PNG, does not matter.
  • The content of the image, such as colors or transparency, does not matter.
  • It seems to be purely an issue of image dimensions.

I encourage you to try using the image in question, which is part of the online SICP book, and see for yourself if you get the same error.

soegaard commented

Hi @pmaddams

There is a new implementation of the sicp picture language available:

https://github.com/sicp-lang/sicp/tree/master/sicp-pict2

Download the rkt-file and the image and save them in the same folder.
Open the rkt-file in DrRacket and run it.
The einstein painter is available as einstein.

Skim though the rkt-file to see the available. For one there are other colors
than black and white. Also images are larger.

/Jens Axel

pmaddams commented

Thanks. I already had this package installed on my system from running raco pkg install sicp. To use it, I renamed the file to "main.rkt."

I think there are two reasonable paths forward:

  1. Make sicp-pict2 available by default with (#%require sicp-pict2). Fix the bug in the older package and retain it for backwards compatibility.
  2. Replace the old code with the new code, so that (#%require sicp-pict) uses the new implementation. For backwards compatibility, provide the old interfaces as wrappers around the new ones. For example, (define load-painter bitmap->painter).

Do you have another solution in mind?

Additional comments:

  • The escher example is not made available with provide.
  • I am not sure the new picture of Einstein is an improvement. The JPEG compression is severely lossy and it is a lower resolution image. The original GIF image looked better.
  • On my machine, the new package takes longer to load, ~9.5 seconds, compared to the old package at ~7 seconds. But this is a small price to pay for a greater assurance of correctness.

Thanks for your work.

soegaard commented

I think 2. is the way to go. Maybe even expect people to use

#lang racket
(require sicp-pict)

The old code was ported from the original mit-scheme (very old) and was made to run with as few changes as possible. Finding bugs in that code is not fun.

Would you be interested in making the changes?
And perhaps find a bigger, higher resolution image?

pmaddams commented

I might be interested in making the necessary changes. However, my previous pull requests were not accepted. I do not want to put the time into understanding and modifying the software, only to eventually find out you want to do it your own way and ignore those changes.

With that in mind, I think it is best if you continue to maintain the software according to your own expectations.

soegaard commented
soegaard commented

It seems I have hit the wrong button. I can't fix though - Github won't let me reopen the pull request because your repository was deleted.

/Jens Axel

sorawee commented

Hi @pmaddams, I'm sorry that the technical difficulties prevented you from contributing to the repo. In any case, since our current random function is based on yours, I added you as a contributor here.

Regarding the vector-ref: contract violation for transforming images of certain sizes error, because we now use the new sicp-pict collection, I believe the bug is fixed, so I will close this issue. Please feel free to reopen the issue if that's not the case.