vydd / sketch

A Common Lisp framework for the creation of electronic art, visual design, game prototyping, game making, computer graphics, exploration of human-computer interaction, and more.
MIT License
1.4k stars 67 forks source link

Double evaluation in defsketch custom bindings #56

Closed Gleefre closed 1 year ago

Gleefre commented 1 year ago

Intro

Hi! As mentioned before in this issue(https://github.com/vydd/sketch/issues/33), initforms of custom bindings in defsketch macro are evaluated twice. (@inconvergent wrote that it seemed to be fixed. Unfortunately, I still observe this issue using both quicklisp version and this repo version.)

Examples

Here is my simple example:

(ql:quickload :sketch)
(use-package :sketch)

(defparameter *result* ())

(defsketch double-evaluation ((hi (push "world" *result*)))
  (text (format nil "~a" *result*) 100 100))

(make-instance 'double-evaluation)

This will display a text (world world) instead of just (world).

As a more complicated example - it seems that Qelt doesn't run properly with current version of sketch because of that.

Solution?

I think that I have found when this issue was introduced - https://github.com/vydd/sketch/commit/19fe20502d26fc75752e710dfe5106ed13345c5a: implementation of make-custom-slots-setf function changed from

(defun make-custom-slots-setf (sketch bindings)
  `(setf ,@(mapcan (lambda (binding)
                     `((,(binding-accessor sketch binding) instance) ,(car binding)))
                   bindings)))

to

(defun make-custom-slots-setf (sketch bindings)
  `(setf ,@(mapcan (lambda (binding)
                     `((slot-value instance ',(car binding)) ,(cadr binding)))
                   bindings)))

Notice that second argument changed from (car binding), which refers to the binding name, to (cadr binding), which refers to the binding initform.

So, the patch proposed in this comment (https://github.com/vydd/sketch/issues/33#issuecomment-627644036) should be the solution to this issue.

inconvergent commented 1 year ago

I must have made a mistake when I checked this the last time, sorry

vydd commented 1 year ago

Thanks! I merged the proposed change.