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

Separate sketch window class #134

Merged
merged 11 commits into from
Jan 26, 2024

Conversation

Kevinpgalligan
Copy link
Contributor

@Kevinpgalligan Kevinpgalligan commented Jan 18, 2024

An attempt to fix the first render being lost due to window resizing (see: #69).

This is achieved by creating a separate sketch-window class so that sketch doesn't have to inherit from gl-window. That way, the slots of sketch can be initialized BEFORE the window gets created, and the window can be assigned the correct dimensions from the start, avoiding a resize.

Implementation notes:

  • Since there may be a circular dependency between the sketch slots and the SDL2 window (specifically: trying to create textures during slot initialization would break because the OpenGL context of the window is not available yet), we have to delay their initialization. My approach is to create a skeleton image object, save a lambda that will do the OpenGL parts of image creation and update the imageobject, and then call all the lambdas after the window has been created.
  • For backward compatibility with sketches that handle input via the kit.sdl2 methods, we have to forward those method calls to the sketch object.
  • For some reason, the call to (gl:clear-color 0.0 1.0 0.0 1.0) in initialize-gl started taking effect after the refactoring, and the background colour of sketches with (copy-pixels t) suddenly became green if they didn't call background anywhere (they had a black background before - I'm not sure why!). So I just changed it to (gl:clear-color 0.0 0.0 0.0 1.0) for backward compatibility.

Various parts of the code, as well as debugging help, were contributed by @Gleefre :)

src/controllers.lisp Outdated Show resolved Hide resolved
src/resources.lisp Outdated Show resolved Hide resolved
(defun init-image-texture! (image surface &key (free-surface t)
(min-filter :linear)
(mag-filter :linear))
(if (delay-init-p)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be more readable & efficient to use a macro like this:

(defmacro maybe-delayed (&body body &aux (thunk (gensym "thunk")))
  `(flet ((,thunk ()
            ,@body))
     (if (delay-init-p)
         (add-delayed-init-fun! (function ,thunk))
         (,thunk))))

And use it like this:

(defun init-image-texture! (...)
  (maybe-delayed
    (let ((texture (car (gl:gen-textures 1)))
          ...)
      ...)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to leave it as-is. My rule of thumb is to use a macro only when necessary. The code could definitely be made more readable, though...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest then to refactor to what would be a macroexpansion of maybe-delayed -- this way the function init-image-texture! will be called only once, also removing an extra call to delay-init-p.

So something like this:

(defun init-image-texture! (...)
  (flet ((init ()
           ...))
    (if (delay-init-p)
        (add-delayed-init-fun! #'init)
        (init))))

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Gleefre here, and the remark agrees with the general codestyle in the repo. No need to fix it immediately though.

src/sketch.lisp Outdated Show resolved Hide resolved
;;; Non trivial sketch writers

(defmacro define-sketch-writer (slot &body body)
`(defmethod (setf ,(alexandria:symbolicate 'sketch- slot)) :after (value (instance sketch))
(let ((win (kit.sdl2:sdl-window instance)))
,@body)))
(when (sketch-%window instance)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest using alexandria:when-let here to avoid calling sketch-%window twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, tried this, but we still end up with alexandria:when-let on top of a let, so I don't think it makes much of a difference.

Copy link
Contributor

@Gleefre Gleefre Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW it could be moved into a forward method:

(defmethod kit.sdl2:sdl-window ((instance sketch))
  (alexandria:when-let (window (sketch-%window instance)
    (kit.sdl2:sdl-window window))

And then also used here:

(defmethod ...
  (alexandria:when-let (win (kit.sdl2:sdl-window instance))
    ,@body))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility would be to use alexandria:when-let* and move the second let there -- fwiw the sdl-window slot could be nil as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(But I don't insist on any of these changes of course; so feel free to resolve the comment thread :)

src/sketch.lisp Outdated Show resolved Hide resolved
src/sketch.lisp Outdated Show resolved Hide resolved
src/sketch.lisp Outdated Show resolved Hide resolved
src/controllers.lisp Outdated Show resolved Hide resolved
src/controllers.lisp Outdated Show resolved Hide resolved
src/sketch.lisp Outdated Show resolved Hide resolved
Comment on lines +140 to +143
;; Make sure there's no garbage functions hanging around.
(loop while (not (zerop (length fs)))
do (vector-pop fs)
do (setf (aref fs (fill-pointer fs)) nil))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to simply create a fresh vector?
Like this:

(setf fs (make-array 0 :adjustable t :fill-pointer t))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, SBCL won't necessarily garbage collect the elements of a complex data structure if they're not manually set to nil. (Happened to me with a massive tree of cons cells, but seems like good practice anyway).

Copy link
Contributor

@Gleefre Gleefre Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that they will be collected alongside the vector though.

@vydd vydd merged commit 1a54e6c into vydd:master Jan 26, 2024
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.

None yet

3 participants