AlexCharlton / cl-glfw3

Common Lisp bindings to GLFW version 3.x
BSD 2-Clause "Simplified" License
89 stars 32 forks source link

removed cffi-libffi dependency #17

Closed tjklemz closed 7 years ago

tjklemz commented 7 years ago

This fixes ARM compatibility for me; see issue #16. But, I need approval on the solution.

Before:

(defcfun ("glfwSetGammaRamp" set-gamma-ramp) :void
  (monitor monitor) (ramp (:struct gamma-ramp)))

After:

(defcfun ("glfwSetGammaRamp" set-gamma-ramp) :void
  (monitor monitor) (ramp gamma-ramp))

By not passing gamma-ramp struct by value, we remove the cffi-libffi dependency. However, it may be possible to pass by value without using the cffi-libffi syntax. Thoughts?

AlexCharlton commented 7 years ago

Hi @tjklemz! Thanks for the PR. I'm actually pretty far removed from this project these days, and CL in general, so I'm afraid I'm unsure of the implications of your changes. I'm all for removing an unneeded dependency, though, particularly one that's causing trouble.

Can you show me what setting a gamma ramp looks like after these changes, and verify that it works for you? As long as this remains possible, I see no reason not to merge.

tjklemz commented 7 years ago

It looks like it works fine. (Essentially, the change means that you have to pass a pointer to set-gamma-ramp instead of passing by value.) I'll test some more tomorrow (it's really late here), but here's my REPL results:

(ql:quickload :cl-glfw3) ; note this is my local-projects version a.k.a. the PR version

(glfw:initialize)

(defparameter m (glfw:get-primary-monitor))
(defparameter r (glfw:get-gamma-ramp m))

(defparameter myred (getf r '%glfw::red))
(defparameter mygreen (getf r '%glfw::green))
(defparameter myblue (getf r '%glfw::blue))
(defparameter mysize (getf r '%glfw::size))

(cffi:with-foreign-object (ramp '(:struct %glfw::gamma-ramp))
    (setf (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::red) myred
          (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::green) mygreen
          (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::blue) myblue
          (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::size) mysize)
    (glfw:set-gamma-ramp m ramp))

Again, I'll write a better test tomorrow, but you can see the values like so:

(defparameter m (glfw:get-primary-monitor))
(defparameter r (glfw:get-gamma-ramp m))
(defparameter g (getf r '%glfw::green))
(%glfw::c-array->list g 10 :unsigned-short) ; where 10 can be anything from 0 to (getf r '%glfw::size)

I'm still learning CL-CFFI, so there might be an easier or different way to create a pointer to a struct. I might play around with trying to pass by value; if anything, it will make me more familiar with all of this.

tjklemz commented 7 years ago

@AlexCharlton I did a more thorough test, and everything is working great. I'm able to change the gamma ramp of the monitor. (IOW, you can test it visually.) I've tested in Clozure CL and SBCL. I've also tested with the GLFW3 C example (from their website docs) in order to compare.

Here's my Lisp test code:

(ql:quickload :cl-glfw3) ; again, this is the PR version in my quicklisp/local-projects folder

(glfw:initialize)

(defun change-gamma ()
    (cffi:with-foreign-objects ((ramp '(:struct %glfw::gamma-ramp)) (r :unsigned-short 256) (g :unsigned-short 256) (b :unsigned-short 256))
        (loop for i below 256
            do (setf (cffi:mem-aref r :unsigned-short i) (* i 64)
                     (cffi:mem-aref g :unsigned-short i) (* i 128)
                     (cffi:mem-aref b :unsigned-short i) (* i 256)))
        (setf (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::red) r
              (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::green) g
              (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::blue) b
              (cffi:foreign-slot-value ramp '(:struct %glfw::gamma-ramp) '%glfw::size) 256)
        (glfw:set-gamma-ramp (glfw:get-primary-monitor) ramp)))

(defun reset-gamma ()
    (glfw:set-gamma (glfw:get-primary-monitor) 1.0))

If you run (change-gamma) your screen will turn bluish. Running (reset-gamma) will change it back to normal. The change-gamma function is just setting a linear ramp of size 256. Obviously, the function could be generalized to take a gamma generator function, etc.

To compare, here is the C version:

#include <GLFW/glfw3.h>
#include <stdio.h>
#include <stdlib.h>

int main (int argc, char *argv[]) {
    char s[100];

    if (!glfwInit()) {
        exit(-1);
    }

    GLFWmonitor* monitor = glfwGetPrimaryMonitor();

    GLFWgammaramp ramp;
    unsigned short red[256], green[256], blue[256];
    ramp.size = 256;
    ramp.red = red;
    ramp.green = green;
    ramp.blue = blue;
    for (int i = 0;  i < ramp.size;  i++) {
        ramp.red[i] = i*64;
        ramp.green[i] = i*128;  
        ramp.blue[i] = i*256;
    }
    glfwSetGammaRamp(monitor, &ramp);

    fgets(s, sizeof(s), stdin); // blocks until you press return

    glfwSetGamma(monitor, 1.0); // resets the gamma

    glfwTerminate();
    return 0;
}

Compile and run this like so (adjusting paths for your machine if necessary):

gcc -o main main.c -lglfw -I/usr/local/include -L/usr/local/lib
chmod u+x main
./main

The program will change the gamma to the bluish hue and wait for you to press enter/return (which will reset the gamma to normal). It should look the same as the Common Lisp version.


Let me know if you have any questions. If this all seems reasonable to you, I think the PR is ready to be merged.

AlexCharlton commented 7 years ago

Sweet! Thanks for the thorough follow up!