You are here

script-fu-render-greebles: basic greeble renderer, feedback requested

Hi! This is my first GIMP script (as well as Scheme program), so feedback and style corrections are welcome!

It's a Greeble renderer - greebles are the random protrusions you find on the surface of starships in science fiction. It selects a bunch of random rectangles, then fills them with the foreground and noises them up a bit based on the active brush. It is based on the method described at http://learnfobia.com/category-Computers-47/tutorial-Painting-A-Spaceshi... .

Screenshot:

Thoughts?

AttachmentSize
greeble.scm7.62 KB

You have posted your script in the Forums section of the Registry. You need to repost it under your Username - Create Content menu under your username on the left vertical sidebar: Username/Create Content/Script

Then apply a license type, and add tags, etc and your script will be accessible by all, front and center on the Registry. Post back if you need help.

I know. It's my first script, so I'm asking for feedback before I post it "officially".

Script fail to complete execution with this error: "Error: eval: unbound variable: gimp-image-select-rectangle". GIMP 2.6.11.
But it should be a useful script for me.

Oh yeah, I'm on the 2.8 nightly. Hold on.

Does this work? I added a compatibility stub for how I think the 2.6 function looks.. http://pastebin.com/PbRVFXcT
(Sorry, don't know how to append files to a response)

[edit] It's also in the Filters->Render menu now.

Working correctly now in Gimp-2.6. :)
Except no color fill or undo.
Thanks!

-Rod

You are off to a good start and you seem to have a good grasp of the functional programming style (it is refreshing to see a script that isn't just a direct translation of how it would be written in a procedural language such as C or Python). Nonetheless, I do have a few thoughts to present for your consideration.

The biggest "problem" I see with your script is the large number of helper functions you've globally defined. While factoring code into small routines is generally a good (great) idea, it can be problematic when writing Script-fus and, if at all possible, these helper functions should be defined within the main procedure.

The problem is that all scripts in GIMP share the same namespace, and any top-level definitions exist in the shared global namespace. There is no "module" mechanism in Script-fu to isolate procedure definitions (there is actually a way to do this in Script-fu that I think might work, but it is rather convoluted and results in some rather atrocious code).

For example, if you and I both have defined in our scripts a procedure called 'blend' then only one of those definition will hold sway; as the scripts are loaded, the second 'blend' definition will overwrite the first. Furthermore, there is no certainty about the order in which the procedures are loaded (barring some extraordinary measures). This is not a problem if the two definitions are functionally equivalent, but if there is any difference then one our scripts will probably fail (and in a way that is very difficult to debug).

Because of this namespace problem, I recommend that you keep the number of globally-defined helper functions to a minimum, and when you do find the need to use them, try to make the names uniquely associated with your script (e.g., your 'greeblestep' procedure is well named in this regard). You can usually avoid introducing global definitions by defining them within your main procedure.

You should include some type of copyright licensing information in your script. It is a sad fact that under the current draconian copyright regime, I have most likely infringed your copyrights by downloading your file so that I could examine it. I would recommend using one of the Open Source licenses such as the GPL or BSD/MIT licenses so that others may freely share your script and customize it to suit their needs.

The remainder of my comments will address matters of convention -- not "problems" per se.

You define a procedure named 'script-fu-channel-grab', which is not registered with the Procedural DataBase. The "script-fu-" prefix is generally reserved for procedures that get added to the PDB. Though you are free to name your functions whatever you wish, this convention provides some instruction to other plug-in/script authors -- and anyone reading your code -- about the functionally of your procedure.

In some cases, you are using the 'list' operator to group a sequence of commands, even though you ignore the result being returned. While this approach works, it is generally preferable to use 'begin' to group commands into a single expression unless you are actually interested in retaining the values returned by each of the individual statements.

Your 'loop' function is very cleverly implemented, but is not really necessary as Scheme provides very similar functionality by using "named let".

(let loop ((count *from*))
  (unless (= count *to*)
    *body*
    (loop (+ count 1)) ))

This obviates the need to wrap your *body* within a lambda; you can just include the statements directly. Note: the symbol that occurs after 'let' does not need to be "loop", it can be anything you wish. Named let supports multiple arguments, and can return a result if desired.

(set! visible-layers
    (let get-visibles-loop ((all-layers (vector->list (cadr (gimp-image-get-layers image))))
                            (visibles '()) )
      (if (null? all-layers)
        (reverse visibles) ; return value - list of visible layers top-to-bottom
        (begin
          (if (zero? (car (gimp-drawable-get-visible (car all-layers))))
            (get-visibles-loop (cdr all-layers) visibles)
            (get-visibles-loop (cdr all-layers) (cons (car all-layers) visibles)) )))))

You have several functions (e.g., 'blend', 'curry', 'combine', 'guard') which generate procedures. Your approach is quite clever, and not out of line with a common Scheme paradigm, however, I would suggest providing these "generators" with names that offer a better indication of their purpose. For example, "gen-curry" or "new-blend" -- something that will instruct the reader of your code what is happening when they are invoked. And again, if these generators are to be defined in the global namespace (per my initial commentary) then they should be named "greeble-gen-curry" or "greeble-new-blend" (or somesuch).

I note also that in some of cases where your procedures take a function as an argument (e.g., the 'with-*' functions), you appear to be doing so merely to be able to "group" multiple steps together (rather than having to pass the function out of necessity). For these situations it would be a better approach to use macros. And again, unless it is impossible to do so, the macros should be defined local to your main procedure, not in the global namespace. Script-fu does not support the syntax-rules of R5RS, but it does support a macro system similar to the old SCM method. Here are some examples:

  (define-macro (with-context . body) 
    `(begin 
      (gimp-context-push) 
      ,@body 
      (gimp-context-pop) ))
  (define-macro (with-undo image . body)
    `(begin
      (gimp-image-undo-group-start ,image)
      ,@body
      (gimp-image-undo-group-end ,image) ))
  (define-macro (with-selection image . body)
    `(let ((channel (car (gimp-selection-save ,image))))
      ,@body
      (gimp-image-remove-channel ,image channel) ))

Your currying of arguments is intriguing, but seems largely unnecessary (especially if some of the preceding forms are used) and tend toward making the code confusing. Of course, that is a purely subjective opinion and even still, I can see where your approach might be beneficial in a larger program.

As a final comment, you have a couple of invocations that use functions that are not available in GIMP 2.6 ('gimp-item-get-image' and 'gimp-image-select-item'). You might consider adjusting those so that your script will work with 2.6.

  ; remove a (temporary) channel after applying it as a selection
  (define (channel-grab img ch op)
    (gimp-channel-combine-masks (car (gimp-image-get-selection img)) 
                                ch 
                                op
                                0 
                                0 )
    (gimp-image-remove-channel img ch) ; not necessary if 'with-selection' macro is used
    )

Many thanks for your extensive reply! I'll update my script to reflect this.

http://paste.pocoo.org/show/555720/

I didn't realize that all script-fu scripts shared the same namespace. I moved all the functions under the main function. I deliberately didn't make use of macros, because I'm not sure if it's possible to define them locally, or how.

The loop function is a deliberate stylistic choice; I don't like having to explicitly write the loop step. I added a license, removed gimp-item-get-image, and added compatibility functions for gimp-image-select-item and gimp-image-select-rectangle, but I haven't tested them on 2.6.

Macros can indeed be local. It was only recently that I was pleasantly surprised to discover this. Before that time, and I've been writing scripts for about 8 years now, I avoided macros because of this concern (that, and the fact that prior to GIMP 2.4 Script-fu did not support macros).

If you prefer your method of looping, implementing it as a macro is not too difficult:

(define-macro (loop sym start end . body)
  `(let xyzzy-iter ((,sym ,start))
    (unless (= (eval ,sym) (succ ,end))
      ,@body
      (xyzzy-iter (succ (eval ,sym))) )))

This form permits you to use any variable name for your index and would be invoked as in the following example:with:

(loop x 1 10 
  (display x) (display " squared is ") (display (* x x))
  (newline) )

As a side note, I chose the rather ugly 'xyzzy-iter' for the loop name because Tinyscheme macros are not hygienic, which means that any occurrence of the loop label within the body would not refer to its value in the user's program, but to the macro's internal label. Macros are one of the few areas where Tinyscheme does not fully support the R5RS specification[1].

I took a quick glance at your update and it appears more understandable now. I would point out that is acceptable (even preferred) to use 'define' within a definition; the only caveat being that all such internal definitions[2] need to appear at the beginning of the outer 'define' (or 'let'-type block). I have not checked yet, but I would presume that any internal macro definitions should similarly appear at the beginning of their enclosing block.

Your use of 'let*' to define your internal procedures is obviously functional, but you could encounter problems if the procedures are created in the wrong order (e.g., there is a forward reference within one of the procedure bodies). For this reason, 'letrec' is preferred, or the internal 'define' previously mentioned (which is merely syntactic sugar for the 'letrec' form).

1 http://people.csail.mit.edu/jaffer/r5rs_toc.html
2 http://people.csail.mit.edu/jaffer/r5rs_7.html#IDX183

gives me nothing but a black layer no matter what settings i use.
I switched back to the other version that left me with a editable selection.

There is also no color fill from foreground and background colors.I am left with just a bunched up set of rectangles with no color. :)

Also there is no undo all

This is on Gimp-2.6

-Rod

Crud. I've had that issue randomly. Try to do select-all before executing the script. (If that works, I'll probably make it the default)

Subscribe to Comments for "script-fu-render-greebles: basic greeble renderer, feedback requested"