Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to add additional superclasses to a sketch. #151

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Kevinpgalligan
Copy link
Contributor

@Kevinpgalligan Kevinpgalligan commented Jan 27, 2024

First draft. Adds a defsketchx macro that allows specifying additional superclasses for a sketch, which means that behaviour can be shared between sketches and it's easier to develop extensions for sketch.

(As per: #142).

Usage example:

(defclass black-background () ())
(defmethod draw :before ((instance black-background) &key &allow-other-keys)
      (background +black+))
(defmethod setup :before ((instance black-background) &key &allow-other-keys)
      (format t "Setting up black background!"))
(defmethod initialize-instance :before ((instance black-background) &key &allow-other-keys)
      (format t "Initializing black background!"))

(defsketchx my-sketch (black-background)
        ((width 200)
         (height 200))
      (circle 100 100 50))

I've also tested out sharing state between a sketch and its superclass, e.g. accessing a superclass slot from the sketch class, and it seems to work as expected, though this requires the user to be know that they can do (slot-value *sketch* 'slot-name) in the sketch body. I don't think that's documented, and I don't think it would work outside the sketch package because *sketch* is not exported.

@vydd
Copy link
Owner

vydd commented Jan 28, 2024

Interesting! Have you considered making it backwards compatible by creating something like

(defsketch foo ((mixins (a b c)))
  (do-stuff))

?

@Kevinpgalligan
Copy link
Contributor Author

Kevinpgalligan commented Jan 28, 2024

I have considered that, might be nicer than having an entirely separate interface. Another possibility:

(defsketch foo (:mixins (a b c)
                :other-option (x y))
   ...)

The only tricky part would be testing whether theres an options list, I guess it should be backward compatible to test whether the first item in the body is a plist.

@vydd
Copy link
Owner

vydd commented Jan 28, 2024

But that's still not backwards compatible, is it? I mean defsketch is, but arg list isn't - so if you want to add mixins to an old sketch, you also need to add the old options under ... :bindings?

I think just using mixins with a list works ok

@Gleefre
Copy link
Contributor

Gleefre commented Jan 28, 2024

What about

(defsketch foo ((:mixins (a b c))
                (some-variable 'hello))
  (do-stuff))

or maybe just

(defsketch foo ((:mixins a b c)
                (some-variable 'hello))
  (do-stuff))

By using a keyword name we show that this parameter is different from usual slots; but the API is kept backward compatible and uniform.

@vydd
Copy link
Owner

vydd commented Jan 28, 2024

@Gleefre what are the cons of using the non-keyword version? is it about the need to figure out what happens when you change it from the code?

@Kevinpgalligan
Copy link
Contributor Author

Kevinpgalligan commented Jan 28, 2024

I might be wrong, but I think my version is backward compatible? The bindings would still be there...

(defsketch my-sketch (:mixins (a b c) :some-future-option 42)
    ((width 400)
     (height 400))
  (circle 100 100 50))

And the code-generating logic would check whether the first item after the name is a plist. If so, treat it as the options plist and the first element of the body becomes the bindings. Otherwise, there are no options, and we treat the item after the name as the bindings. Like so:

 (defun make-sketch-class-def (name options-or-bindings body)
   (if (is-plist-p options-or-bindings)
       (let ((options (parse-options options-or-bindings))
             (bindings (parse-bindings (car body)))
             (body (cdr body)))
        (make-def name options bindings body))
       (let ((options nil)
             (bindings options-or-bindings))
         (make-def name options bindings body)))

I'm also open to making the configuration options part of the bindings list, of course. Depends on which option ye think is neater.

And yeah, I think the point of using keyword args is that they're easily distinguishable from regular bindings and should be backward compatible in case someone happens to have a slot called mixins.

@Kevinpgalligan
Copy link
Contributor Author

I went with your first suggestion, @Gleefre ! Now options like :mixins can be passed at the start of the bindings form.

My example usage is unchanged, except it now looks like:

(defsketch my-sketch
        ((:mixins (black-background))
         (width 200)
         (height 200))
      (circle 100 100 50))

As expected, it generates the following code:

(DEFCLASS MY-SKETCH (SKETCH BLACK-BACKGROUND) NIL)

Oh, I forgot to update the documentation.

@Gleefre
Copy link
Contributor

Gleefre commented Jan 28, 2024

what are the cons of using the non-keyword version? is it about the need to figure out what happens when you change it from the code?

It is an interesting question what should happen if mixin list is changed... Dunno.

My point of using keyword is to make it more readable -- it hints that this is not a slot (since you can't have variables named by keywords); but an additional option to the macro itself.

As suggested by Gleefre, the :mixins binding form now looks
like (:mixins m1 m2) instead of (:mixins (m1 m2)), 'cause it
looks neater. Also, it can appear anywhere in the list of
binding forms, not just at the start.

(sketch-slot-value 'slot-name) can be used to access slots that
are introduced by the mixin. Previously, I'd exported *sketch*
so that users could do (slot-value *sketch* 'slot-name), but that
seems to expose too much implementation detail.
@Kevinpgalligan
Copy link
Contributor Author

Kevinpgalligan commented Oct 31, 2024

Updated! Copying the commit message to describe the change.

As suggested by Gleefre, the :mixins binding form now looks like (:mixins m1 m2) instead of (:mixins (m1 m2)), 'cause it looks neater. Also, it can appear anywhere in the list of binding forms, not just at the start.

(sketch-slot-value 'slot-name) can be used to access slots that are introduced by the mixin. Previously, I'd exported *sketch* so that users could do (slot-value *sketch* 'slot-name), but that seems to expose too much implementation detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants