Gleefre/rewrite defsketch

[Gleefre]

Rewrites the defsketch macro.

These are notable changes:

• prepare and draw methods now use *sketch* as their argument, so the sketch::instance lexical variable is no longer captured.
• prepare method now computes slots values by providing the default value form for the key arguments. The could've had effect on updating obsolete instances, but in fact in this case initargs argument will be always null (see ANSI 4.3.6.3), so the behavior is not changed.
• prepare no longer has custom method-combination (progn).
• The :initarg and :accessor can be specified in the binding to control defined slots. The syntax for the bindings is now either name or (name &optional value &key initarg accessor)
• channels support [in defsketch] was dropped, as they were not documented and were full of bugs / edge-cases anyways. I plan to replace their API later by using a key argument in the binding alike :initarg or :accessor.

[Gleefre]

To elaborate on channels:

One issue is global environment: Ideally "channels" should be window-specific, as well as "observers".

Another issue is with observers - they are currently defined at macroexpansion time, which doesn't make sense, especially given the first issue.

The third one is the concept - in fact they are just variables in the "channel" namespace with hooking mechanism, i.e. with ability to call a function after the variable was set; so their name is kind of misleading.

To remove semi-broken codewalker you could simply specify on which variables updates should the hook be called.

[Gleefre]

Accidentally closed, reopening.

[Gleefre]

How do I know that this is not introducing new problems?

This is a valid concern. To my defense I can say that I did test it on multiple programs I wrote before as well as I tested it on a small example with multiple sketch redefintions.

it's absolutely not clear if it calls for a rewrite

I suppose I did not describe that, yes. I'm doing this rewrite mostly as preparation work before splitting backend (sdl2 / opengl) from the core of the sketch; but also to make the source code somewhat less complicated.

Have you considered a less invasive solution?

I'm sorry that this PR is invasive, but I wasn't able to figure this one out - calling prepare method needs the instance slots to be initialized, but setting width / height of the window beforehand can only be done by passing key arguments to call-next-method where it is used by kit.sdl2:gl-window. However this is not possible in all cases because height / width calculations might depend on earlier bindings; so it can only be known after calling the prepare method. One possible solution would be to move the underlying window to the sketchs slot instead of inheriting it, and another one would be to move calculations done in prepare to the initialize-instance :around method, and also to the update-instance-for-redefined-class methods; but both require the defsketch macro not being bloated and hard to modify (which it is to me in its current form).

I agree that automated tests would be neat, though I'm not sure how to go about that - I don't know how to create automatic testing for something that involves application.

it's bad form to just completely drop channels

I agree, although as I described above currently they are not working well from my experience. My main concern about them is usage of compile-time effects on the global environment from a macro, which doesn't really work well.

About benefits of these change: All parsing of bindings is happening in one place, so it is easy to add / remove functionality. prepare and draw methods were simplified due which was made possible by the first one.

In any case, feel free to close or ignore this PR.

[Gleefre]

I think it should've been obvious that I still feel like there's something in that idea.

Yes, it is! And I am quite interested in it too. It is, however, kind of broken right now (at lease I think it is), so I probably would like to rewrite them too in a more "honest" way where they are using a namespace library, for example in-nomine.

[vydd]

Thanks for continuing the conversation!

In any case, feel free to close or ignore this PR.

I don't want to ignore it, as there is good things in it for sure. I would actually like to accept a lot of fixes you are suggesting, but having all or nothing PRs makes things hard.

As an example, I've extracted the fix for sketch::instance capture into a new (co-authored) PR. I would much rather merge something like that, the binding struct, setter :after, and the thing related to width/height that I still don't understand fully. I would probably reject channel changes for now, and I'm divided on the rewrite of defsketch - yes, it's less code in the macro, but more paths to follow. Right now I'm finding some utility in having it represent a rough shape of what's generated - defclass, prepare, draw.

I'm doing this rewrite mostly as preparation work before splitting backend (sdl2 / opengl) from the core of the sketch

Not saying this shouldn't be done - it should, just in terms of cleaner design, but I'm curious, what new backend are you thinking of? Do you see problems down the line with maintaining multiple backends? What I would love to do for example is integrate Sketch with CLIM somehow.

[Gleefre]

what new backend are you thinking of?

One example of what I have somewhat successfully able to do is to embed the sketch into gtk's GlArea. I don't have a very clean way to do that yet though.

Another possible backend could be web-based (maybe WebGL), so that it would be possible to make browser applications :). Although this one is just hopes right now.

[Gleefre]

I would actually like to accept a lot of fixes you are suggesting, but having all or nothing PRs makes things hard.

I would be happy to assist - but I'm not sure which things you'd be interested in - and splitting a rewrite is quite hard actually. So feel free to make a list (?) of changes you'd like to have in separate PRs and I'll try to do that :)

[Gleefre]

the binding struct, setter :after, and the thing related to width/height

Oh, missed that.

Separate PR for:

• [ ] binding struct
• [x] setter :after

Actually, there isn't any changes related to width / height in this PR.

I'll try to outline other changes in this list too

• [x] Move parts of defsketch to separate functions:
  • [x] Move draw definition to a separate function
  • [x] Move prepare definition to a separate function
  • [x] Move the class definition to a separate function
• [ ] Move let* bindings in prepare to the &key lambda list ( that allows to simplify it a lot )

Then it also removes unneeded helpers; and channels support is dropped due to me not liking the syntax used (the binding must be in the form (in [channel-name] <value>), and then the <value> is used as the initform everywhere. I could re-add it in the current form channels have (even though I dislike it and think it is buggy due to using compile-time effects); maybe change the syntax from (in [channel-name] <value>) to (<value> :observer [channel-name])? ( or another keyword )

[Gleefre]

The binding struct is somewhat harder to separate as merge conflicts with #90 and #88 may arise. (Also there might be a merge conflict between #90 and #88 - I could rebase & resolve those if they arise.)

[vydd]

Actually, there isn't any changes related to width / height in this PR.

that's interesting, because when you said something's wrong with width/height, I started doing random experiments, and found that neither can be changed at runtime with the current code, but they work fine starting from 535f716 .

This is the sketch I used

(defsketch foo
    ((width 100)
     (height 100)
     (a 0))
  (incf a 0.01)
  (setf height 150)   ; I used this to change the value from the code buffer and recompile
  (setf width (truncate (* 100 (+ 2 (sin a))))))

So the fix is something that's absolutely welcome. Again, I'm not saying defsketch rewrite is not, but there's a big chunk that's in my opinion a matter of personal preference - and I'm much more interested in fixing bugs, extending existing features, and building new ones, instead of rewriting something that already works reasonably well.

[Gleefre]

Ah, I see what you mean. It is not the width/height issue, but the issue with standard slots (at least that's how I identify it - I thought you were talking about default width/height to avoid resizing the window on startup). It is indeed resolved in the "binding structure" part, where it uses parsed-bindings in the draw method

[vydd]

One possible solution would be to move the underlying window to the sketchs slot instead of inheriting it

This is probably a good idea, btw

[Gleefre]

Actually, there is one more change here:

• [x] Remove non-standard method combination for prepare

[vydd]

removing progn method combination for prepare - I probably didn't understand it was going to be internal, or I had other plans for it that I've now forgotten

https://github.com/vydd/sketch/pull/91

[Gleefre]

Resolved merge conflicts. Also:

• restored channels support;
• rewrote binding struct as a class.

Minor fixes:

• added a fix for setting env-y-axis-sgn in the correct plac;
• removed unneeded declarations.