You are here

Error: car: argument 1 must be: pair

Hi, can anybody give me a hand? I have a script I'm trying to de-bug but it's like stumbling around in a coal shed looking for a black cat in the dark. I've managed to fine down the trouble area to a copying loop which selects by colour and pastes into a new layer. It does this for every colour so can build up quite a number of layers before it terminates. Some days it'll run 100 times out of 100 but sometimes it aborts part way through with the empty list error "Error: car: argument 1 must be: pair."

There appears to be no consistency in where it will stop - it could be on layer 4 or layer 40. I've tried it on several computers all running Gimp 2.6 and the frequency does seem to possibly be hardware related in that it fails more frequently on one computer. CPUs and memory are all similar spec.

If I was writing this in machine code I'd be looking for a stack overflow but with a script I just don't know where to look.

I would have thought that if a script runs once, it ought to run every time but it obviously isn't that simple. Are there instances where the "car" error can be thrown unpredictably?

Any assistance gratefully received.

Cheers,

Adrian.

I think this is a bug.

I have run into it before but can't find the workaround...
It involved sampling colours... OK found it:


(set! nextcolour (car (gimp-palette-entry-get-color inPalette (rand numcolours)))) ; random palette colour
(while (or (< (length nextcolour) 3) ; bug
(set! nextcolour (car (gimp-palette-entry-get-color inPalette (rand numcolours)))) ; random palette colour
)

So I found that the call to gimp-palette-entry-get-color inPalette didn't always return a triplet, and later calls would error, so I added the while check to keep trying until it worked.

Hope that helps.

-Rob A>

Hi Rob,

Thanks very much for your input - that makes perfect sense.

Cheers,

Adrian

Hi again Rob,

Thanks for your help and for pointing me in the right direction.
I tried using a "while" loop as you suggested but I must have had it in the wrong place because the script still failed after several hours. By this time I'd done some digging on the Gimp site and had found your 2009 conversation with Charles Belov about the same bug. He seemed to be doing exactly the same thing as I am trying to do so I pinched his idea of a safe wrapper and modified it to suit my script. Negative proof is no proof at all but so far it seems to have cracked it. I've run it over one hundred times without any errors but it could yet turn out to be false dawn. There is a downside to using the wrapper in that it doubles the run time of the script which was already over long. In retrospect, I should have written it as a plug-in. Working at pixel level, it runs through thousands of iterations and a compiled version would be much better but I've never programmed in C and am only now getting to grips with scripts. Maybe next week.

Cheers,

Adrian

If you post the code of your script, maybe some of us could suggest some optimizations.

Hi Rob,

I don't doubt it could do with some optimizations!

In 2009 I modified a few older scripts I'd downloaded which wouldn't work with the then new version of Gimp and I wrote a couple of short routines which I needed at that time but since then I've done nothing until my girlfriend wanted a plug-in to help her make a tapestry. She's been using it for a few weeks and as it only fails on odd occasions we were prepared to live with it but I thought I'd upload it to the plug-in registry so it needed to be made bomb proof first. I couldn't get to grips with what was happening with the colour picking so asked for advice. It now seems to run but probably contains stuff which I no longer need and maybe doesn't contain stuff which I ought to have. Basically, it's just a series of PDB calls to automate what she was doing manually.

Any suggestions gratefully received.

Cheers - Adrian

;
; -*-scheme-*-
;
; Script-Fu Tapestry v2.2
; Janet Fulcher and Adrian Spriddell - February 2011
;
; A Gimp script which breaks down an image into cross stitch colours.
;
; This software is in the public domain.
;
; I have released this script into the wild in the hope that it may
; prove useful. It comes with absolutely no warranty whatsoever,
; without even the implied warranty of merchantability or fitness
; for any particular purpose. See the GNU General Public License for
; details. You may redistribute and/or modify this script or extract
; segments from it without prior consent.

;
; Tested with Gimp-2.6. GIMP uses the tiny-scheme interpreter.
; The Gimp-specific functions are made available to scripts via the
; PDB (Procedural Data Base). You can look up functions in the GIMP
; with the DB-Browser, available from the Script-Fu menu.
;
; NEEDS TO HAVE THE PATTERN "Tapestry5x5" PRE-INSTALLED IN THE GIMP.
;
; Located in the Gimp menu.
;
; "Tapestry" makes a fresh image to work on with the registration and
; pattern grids in separate layers so that they may be turned off and on as
; required. Floss colours are optionally converted to individual layers.
;
; If your source image contains layers, make sure the background
; layer is set "active" or the copy process could produce some odd results.
;
; Depending on your source image, it may be a good idea to crank up the
; brightness and contrast levels before you start and to scale to the
; size of your canvas. Count the number of stitches to the inch your
; chosen canvas provides - say 14 stitches per inch. There are 5 pixels
; to a stitch so 70 pixels per inch. For a canvas 24 by 32 inches,
; that's 24x70 by 32x70 = an image size of 1680x2240.
;
; Be sure to save your final image in xcf, bmp or other uncompressed
; format. DO NOT SAVE AS A JPEG - the colours will be much too blurred.
;
; Change-log:
; v2.0 Added registration and guide grids
; v2.1 Added option to convert colours to layers
; v2.2 Added Safe Wrapper by Charles Belov to trap colour pîcker bug
;
; To-Do list:
; Option to add symbols to each colour - easier to see in hard copy

(define (script-fu-tapestry image drawable Colourdepth Colour2layers?)

(let* (

(x 0)
(y 0)
(line 0)
(extent 0)
(pattern 0)
(grid 0)
(value1 0)
(value2 0)
(value3 0)
(colour 0)
(nextlayer 0)
(background 0)
(width (car (gimp-drawable-width drawable)))
(height (car (gimp-drawable-height drawable)))

)

; Ensure the image size variables are in multiples of 10
; so the grid will be overlaid correctly.

(set! height (round (/ height 10)) )
(set! width (round (/ width 10)) )
(set! height (* 10 height))
(set! width (* 10 width))

; Shove the current environment on the stack for retrieval later

(gimp-context-push)

; Make sure there are no active selections then copy all the
; visible layers of the source image so we don't mess it up.

(gimp-selection-none image)
(gimp-edit-copy-visible image)

(set! image (car (gimp-edit-paste-as-new)))
(set! drawable (car (gimp-image-get-active-drawable image)))

(gimp-display-new image)

(gimp-image-undo-disable image)

; Scale the new image dimensions if necessary.

(gimp-image-scale image width height)

; Pixelise to five pixel squares.

(plug-in-pixelize RUN-NONINTERACTIVE image drawable 5 )

; Now Posterise to set the maximum number of floss colours.
; (You may need to experiment a bit with this setting).

(gimp-posterize drawable Colourdepth)
(set! background (car (gimp-image-get-active-layer image)))
(gimp-displays-flush)

(when (equal? Colour2layers? TRUE)

; Clear selections

(gimp-selection-none image)

; Clear any highlights so I can fill with white after cutting

(gimp-levels drawable 0 0 255 1 0 254)

(set! drawable (car (gimp-image-get-active-drawable image)))

(gimp-context-set-default-colors)

; SAFE WRAPPER FOR COLOUR PICKER BUG
; DOES NOT ALWAYS RETURN A TRIPLET SO KEEP TRYING TILL IT DOES

(define (safe-gimp-image-pick-color image drawable x y FALSE FALSE 0)

(let* ( (colour (car (gimp-image-pick-color image drawable x y FALSE FALSE 0))))

(while (< (length colour) 3)

(set! colour (car (gimp-image-pick-color image drawable x y FALSE FALSE 0))) )

(list colour) ) )

; END OF SAFE WRAPPER

; Start copying loops

; Line loop
(let loop ((y 2))
(if (< y height)
(begin

; frame loop
(let loop ((x 2))
(if (< x width)
(begin

; Get the colour of the first pixel and make a new layer from it

(set! colour (car (safe-gimp-image-pick-color image drawable x y FALSE FALSE 0))
(cdr (safe-gimp-image-pick-color image drawable x y FALSE FALSE 0)) )

; If the pixel colour is white, it's already been picked so . . .
; Get value of pixel colour to check

(set! value1 (car colour))
(set! value2 (cadr colour))
(set! value3 (caddr colour))

(if (= (+ value1 value2 value3) 765)

(begin

; In this event, it's white so do nothing

)

(begin

; Otherwise .... make a new layer

(gimp-by-color-select drawable colour 0 CHANNEL-OP-REPLACE FALSE FALSE 0 FALSE)

(gimp-edit-cut drawable)

(gimp-edit-fill drawable BG-IMAGE-FILL)

(set! nextlayer (car (gimp-image-get-selection image)))

(car (gimp-drawable-is-layer drawable))

(gimp-floating-sel-to-layer (car (gimp-edit-paste drawable 1)))

) )

(loop (+ x 5)))))

; Reached the end of the line so start on the next one

(loop (+ y 5)))))

; Run out of lines and stuff to copy
; Cancel any active selections

(gimp-selection-none image)

) ; End of conditional Colour2layers block

; Make a new layer "pattern" with an alpha channel.

(set! pattern (car (gimp-layer-new image width height
RGBA-IMAGE "Pattern Layer" 100. NORMAL-MODE)))

(gimp-image-add-layer image pattern -1)
(set! drawable (car (gimp-image-get-active-drawable image)))
(gimp-drawable-fill pattern TRANSPARENT-FILL)

; Overlay a pattern of 5x5 squares.

(gimp-context-set-pattern "Tapestry5x5")
(gimp-edit-fill drawable PATTERN-FILL)

; Make another new layer "grid" with an alpha channel.

(set! grid (car (gimp-layer-new image width height
RGBA-IMAGE "Grid Layer" 100. NORMAL-MODE)))

(gimp-image-add-layer image grid -1)
(set! drawable (car (gimp-image-get-active-drawable image)))
(gimp-drawable-fill grid TRANSPARENT-FILL)

; Set the foreground and background colours

(gimp-context-set-foreground '(255 255 0))
(gimp-context-set-background '(255 50 0))

; Here we draw a grid of red horizontal reference lines
; on the image one pixel wide every 10 squares.
; Starting 50 pixels offset centre and progressing downward.

(set! line (+ (* height 0.5) 50))
(set! extent height)

(while (< line extent)
(gimp-rect-select image 0 line width 1 REPLACE 0 0)
(gimp-edit-fill drawable BG-IMAGE-FILL)
(set! line (+ line 50)))

; Then start at an offset centre and progress upward.

(set! line (- (* height 0.5) 50))
(set! extent 0)

(while (< extent line)
(gimp-rect-select image 0 line width 1 REPLACE 0 0)
(gimp-edit-fill drawable BG-IMAGE-FILL)
(set! line (- line 50)))

; Now draw a similar grid of vertical lines.

(set! line (+ (* width 0.5) 50))
(set! extent width)

(while (< line extent)
(gimp-rect-select image line 0 1 height REPLACE 0 0)
(gimp-edit-fill drawable BG-IMAGE-FILL)
(set! line (+ line 50)))

(set! line (- (* width 0.5) 50))
(set! extent 0)

(while (< extent line)
(gimp-rect-select image line 0 1 height REPLACE 0 0)
(gimp-edit-fill drawable BG-IMAGE-FILL)
(set! line (- line 50)))

; Select and fill a yellow vertical line 1 pixel
; wide centered on the currently active drawable.

(gimp-rect-select image
(+ x (* width 0.5))
(+ y (* height 0.0))
(/ width width)
(* height 1.0)
CHANNEL-OP-REPLACE 0 0)

(gimp-edit-fill drawable FOREGROUND-FILL)

; Select and fill a yellow horizontal line 1 pixel
; wide centered on the currently active drawable.

(gimp-rect-select image
(+ x (* width 0.0))
(+ y (* height 0.5))
(* width 1.0)
(/ height height)
CHANNEL-OP-REPLACE 0 0)

(gimp-edit-fill drawable FOREGROUND-FILL)

; Do the housekeeping

(gimp-image-set-active-layer image background)
(gimp-selection-none image)
(gimp-image-undo-enable image)
(gimp-context-pop)
(gimp-displays-flush)

) ; End of let block
) ; End of definition block

; Here endeth the procedures.

; Register the function in the GIMPs PDB.

(script-fu-register "script-fu-tapestry"
_"/Filters/Artistic/_Tapestry..."
"Breaks down an image into cross-stitch colours. Needs to have the pattern Tapestry5x5 installed"
"Micomicon"
"Janet Fulcher & Adrian Spriddell"
"January 2011"
"RGB* INDEXED*"
SF-IMAGE "Input Image" 0
SF-DRAWABLE "Input Drawable" 0
SF-ADJUSTMENT "Posterize Levels (to set the number of floss colours)" '(5 2 8 1 8 0 0)
SF-TOGGLE "Make a new layer for each colour (this could take several minutes)." 0

)

When posting code, if you wrap [pre][/pre] tags around it (substituting less-than and greater-than characters for the square brackets) then it will be presented with the original formatting intact.

One trivial speedup would be to remove the 'cdr' expression from the following code; it is ignored by the interpreter (it is like (set! x 1 2) in that the "2" is completely ignored).

(set! colour (car (safe-gimp-image-pick-color image drawable x y FALSE FALSE 0))
             (cdr (safe-gimp-image-pick-color image drawable x y FALSE FALSE 0)) )

One significant speedup to your code would be to use 'plug-in-grid' to render your grid lines, rather than manually selecting strips and filling them. It takes a little bit of effort to learn the parameters but in additional to speeding things up it would make your code much "cleaner".

Your script currently posterizes the original image to a maximum of 512 colors (8*8*8). If you were willing to accept 256 colors as your upper limit, a potential speed increase might be obtained by using 'gimp-image-convert-indexed' to create a quantized palette of all of the colors. This palette could then be used to create separate layers for each color, without having to resort to examining the color values in the image and without concerning yourself whether a particular cell has already been processed.

A few years back, I wrote a script which employs this separation of indexed images into layers (http://flashingtwelve.brickfilms.com/GIMP/Scripts/indexed-decompose.scm) and I would be glad to help you incorporate the approach if you find the 256-color limitation acceptable.

Hi Rob,

Thanks for taking the trouble to review my code and for your suggestions. I didn't know about the pre tags. I think an upper limit of 256 colours would still be very generous as I'm told that usually no more that 30 would be needed. I'm now going to investigate the plug-in-grid and your script.

Thanks again - your help's really appreciated - Adrian

Here is some code which performs the separation of the colors using 'gimp-image-convert-indexed'.

I would recommend against using Posterize because it can change the original colors much more drastically (indexing will use the original colors as much as possible). If you do indeed want to use Posterize, just do it before running 'separate-colors'. If you want to have your tapestry mapped to a predefined palette of colors (but not the ones predefined by Posterize), you can do that by modifying the call to 'gimp-image-convert-indexed'.

Note that all of the work is done on a temporary image (work-image) and then the resulting layers are transferred back to the original 'image'. This permits the original image to retain its mode and does not slow things down (duplicating an image is quite fast as long as you don't create a new display). I left out displaying progress bars because it would've made the code less readable; but you might consider adding them.

Each of the layers are named using its constituent colors (e.g., "RGB=[124, 57, 35]"). You may wish to change this (especially if you are using a predefined palette with properly named colors).

Be aware that running this function with a large 'number-of-colors' on a large image can quickly eat up memory. For example, 255 layers for a 2000x1000 pixel image will be well over 2 gigabytes. This is not a problem for fewer colors or for smaller images (or if you have lots of available memory).

(BTW, I'm not Rob. I don't mind the association, but Mr Antonishen might not consider it flattering :) )

   (define (separate-colors image number-of-colors)
      (let* ((work-image (car (gimp-image-duplicate image)))
             (layer (car (gimp-image-get-active-layer work-image)))
             (colors '())
             (layers '()) )
        (gimp-image-undo-disable work-image)
        (unless (= (car (gimp-image-base-type work-image)) INDEXED)
          (gimp-image-convert-indexed work-image NO-DITHER MAKE-PALETTE number-of-colors 0 TRUE "") )
        (set! colors (vector->list (cadr (gimp-image-get-colormap work-image))))
        (while (pair? colors)
          (let ((new-layer (car (gimp-layer-copy layer TRUE)))
                (red (car colors))
                (green (cadr colors))
                (blue (caddr colors)) )
            (gimp-by-color-select layer
                (list red green blue) ;; color
                0 CHANNEL-OP-REPLACE FALSE 0 0 0 ;; Threshold=0, NO Feather, Layer Only
                )
            (gimp-drawable-set-name new-layer
                (string-append "RGB=[" (number->string red) ", " (number->string green) ", " (number->string blue) "]")
                )
            (gimp-image-add-layer work-image new-layer -1)
            (gimp-selection-invert work-image)
            (gimp-edit-clear new-layer) 
            (set! layers (cons new-layer layers))
            (set! colors (cdddr colors)) ))
        (while (pair? layers)
          (let ((new-layer (car (gimp-layer-new-from-drawable (car layers) image))))
            (gimp-drawable-set-name new-layer (car (gimp-drawable-get-name (car layers))))
            (gimp-image-add-layer image new-layer -1)
            (set! layers (cdr layers)) ))
        (gimp-image-delete work-image)
        ) )

Hi Saul,

Apologies for getting your name wrong - I hadn't noticed the switch. Thanks for taking the trouble to help me. I tried your script - yep way better. Runs at four times the speed and after experimenting I agree that indexing is a lot better than posterizing. Learning all the time.

I've re-written my script in the way you suggested and am pleased with the result. I couldn't use the plug-in grid to draw the reference grid because I couldn't find any way to centralise the thing but I have used it to make the pattern grid which has enabled me to get rid of the necessity to bundle the pattern with the script.

One question perhaps you would answer for me: I haven't been able to figure why the resulting image is four times the size as it was using my old script.

Now I'm going to study your latest script - thanks for posting that. I've only just seen it as I've been travelling all weekend and only just now got back on line.

Cheers, Adrian

> One question perhaps you would answer for me: I haven't been able
> to figure why the resulting image is four times the size as it was
> using my old script.

Does this occur even if you don't separate the colors to separate layers? If it happens only when my code is executed then you might need to set the layer offsets after the new-layer has been copied from the 'work-image' to the original 'image' (in the last "while" loop).

Hi Saul, Only happens when copying to layers I think but I will investigate further.

Right - using my test image which is a 640x480 bmp I get the following results:

Using my original "colour-picker" script:
Making 65 layers = saved xcf image size of 3.6meg
Making 96 layers = saved xcf image size of 5.0meg
Not making layers - 96 colours = xcf of 473k

Using the method from your first decompose script:
Making 65 layers = saved xcf image size of 16.3meg
Making 96 layers = saved xcf image size of 25.5meg
Not making layers - 96 colours = xcf of 711k

New script - looks like it does the same thing but by a different route. Questions arising:
What is the advantage of using vector>list instead of map?
What is the advantage of using a work image and copying back rather than just working on a copy. I still haven't got my head around that.

Not run any actual tests with it yet as right now I have an argument error (must have copied something wrongly).

Thanks for your patience - Adrian

Response to "February 9, 2011 - 13:46 — micomicon" (replies were nesting too much for my taste)

> What is the advantage of using vector>list instead of map?

vector->list is being used to convert the array of layer IDs to a list of layer IDs; this would still be necessary if 'map' were used to loop through the list instead of a 'while' block. I chose to use a 'while' loop (instead of 'map') for two reasons: I thought it would be more clear to someone new to Script-fu and using 'map' makes it a bit difficult to implement progress bars.

If I were coding for myself, I'd probably use a "named let" to loop through all of the layers. The rule-of-thumb I currently use is:

Use 'while' if looping through non-lists.

  (while (< x 50)
    ...
    (set! x (+ x 1)) )

Use 'map' if looping through a single list and there are no other loop variables.

  (map gimp-drawable-get-name '(layer1 layer2 layer3))

Use 'named let' for more complex looping through multiple lists, looping variables, or test conditions.

  (set! target-position (car (gimp-image-get-layer-position image active-layer)))
  (let loop ((layers (vector->list (cadr (gimp-image-get-layers image))))
             (above-layers '())
             (below-layers '()) )
      (if (null? layers)
        (list above-layers below-layers) ; return a list of two lists
        (let ((layer (car layers))
              (current-position (car (gimp-image-get-layer-position image (car layers)))) )
          (cond 
            ((= current-position target-position)
              (loop (cdr layers) above-layers below-layers) )
            ((< current-position target-position)
              (loop (cdr layers) (cons layer above-layers) below-layers) )
            ((> current-position target-position)
              (loop (cdr layers) above-layers (cons layer below-layers)) )))))

> What is the advantage of using a work image and copying back
> rather than just working on a copy. I still haven't got my head
> around that.

The basic reason for doing the quantization on a separate image is because there is no "convert layer to indexed" command in the PDB and converting the entire image to indexed could result in irreversible changes being made to other layers. In your specific case -- as it is currently implemented -- this is not necessarily a problem, since your script is already doing its work on a newly created image and that new image only has one layer. Nonetheless, a more general approach is not a bad idea; it will facilitate improvements and reduce the potential for hard-to-detect problems in the future.

Hi Saul,

Thanks very much for the answers. I'll paste them into my Script-Fu lessons manual for future reference. I'm not very good with high level languages. By the way, I added the file sizes onto my list of questions - you probably haven't seen them yet.

Thanks again for all your help.

Regards - Adrian

The large file sizes are to be expected with the current implementation. Basically, how much memory a layer uses is determined by the layer's dimensions; an extracted color layer that is entirely transparent except for one pixel will require just as much memory as your original layer.

You can potentially reduce the file size by performing an autocrop on the color layer after copying it from the work image to your final image.

Be aware that 'plug-in-autocrop-layer' does not behave as one might initially expect. The filter will modify the active layer based upon the contents of the layer passed as the third argument to the procedure. To avoid insidious problems when using the filter, it is a good idea to explicitly activate the layer you want changed before calling the procedure.

(gimp-image-set-active-layer image target-layer) ; target-layer is the layer being resized
(plug-in-autocrop-layer RUN-NONINTERACTIVE image source-layer) ; source-layer is usually the same as the layer being resized

This is a feature (not a bug), but it is not documented very well (and is somewhat unintuitive).

Note: a slightly better solution would be to autocrop your extracted color layers before copying them from the work image (this will preserve more of your UNDO history); though if you do this then you'll have to change the offsets of the copied layers to match the offsets they had in the work image. This is not particularly important and I don't think you should worry about such optimizations at this point in your scriptwriting.

Hi Saul,

I'm not that bothered about the file size but was curious as to why there was such a difference. Thanks for the explanation - I think it's brilliant that you guys are prepared to spend much of your time trying to sort out idiots like me who don't know which way's up!

Cheers, Adrian.

Subscribe to Comments for "Error: car: argument 1 must be: pair"