Glu Tessellation support #20

Closed
wants to merge 25 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

dardoria commented Feb 15, 2011

Hi,
I implemented the functions for polygon tessellation. I would be glad if you could consider merging them in cl-opengl.

Thanks
Boian

@3b 3b and 1 other commented on an outdated diff Feb 23, 2011

;;;; 5.4 Control Over Tessellation
-
-;;(defcfun ("gluTessProperty" tess-property) :void
-;; )
+(defcenum (tess-property %gl:enum)
+ (:tess-winding-rule 100140)
+ (:tess-boundary-only 100141)
+ (:tess-tolerance 100142))
+
+(defconstant +tess-winding-odd+ 100130)
@3b

3b Feb 23, 2011

Owner

The +tess-winding-...+ constants should probably be a defcenum, and i would drop the tess- prefix unless they are passed to something where there might be a conflict, maybe even drop the whole tess-winding-, if it is usually used with :tess-winding-rule or otherwise obvious from context.

@dardoria

dardoria Feb 24, 2011

Contributor

Done

@3b 3b commented on the diff Feb 23, 2011

glu/glu.lisp
;;;; 5.3 Callbacks
-;;; TODO
-;;(defcfun ("gluTessCallback" tess-callback) :void
-;; )
+(defcenum (tessellation-type %gl:enum)
@3b

3b Feb 23, 2011

Owner

I probably would drop the tess- prefix from these, if they only get passed to tessellation specific functions

@dardoria

dardoria Feb 24, 2011

Contributor

done

@3b 3b commented on the diff Feb 23, 2011

glu/glu.lisp
;;;; 5.4 Control Over Tessellation
-
-;;(defcfun ("gluTessProperty" tess-property) :void
-;; )
+(defcenum (tess-property %gl:enum)
@3b

3b Feb 23, 2011

Owner

same comment about tess- prefix as tessellation-type

@dardoria

dardoria Feb 24, 2011

Contributor

done

@3b 3b commented on the diff Feb 23, 2011

glu/glu.lisp
@@ -456,7 +486,14 @@
:out-of-memory
:incompatible-gl-version
:invalid-operation
- ;; plus NURBS, Quadrics and Tesselation errors?
+ ;;Tesselation errors
+ (:tess-missing-begin-polygon 100151)
@3b

3b Feb 23, 2011

Owner

tess- prefix on these seems reasonable though, since it is a more generic enum

@3b 3b and 1 other commented on an outdated diff Feb 23, 2011

glu/interface.lisp
+ ,@callback-body)
+ `(defcallback ,tessellation-cb :void ,(cdr args)
+ (,name *active-tessellator* ,@arg-names)))
+ (push (make-tess-callback :name ,tessellation-name :generic-function #',name
+ :callback ',tessellation-cb :callback-type ,callback-type
+ :arg-count ,(length args))
+ *tess-callbacks*))))
+
+(defmacro define-tessellation-callbacks (&body callback-specs)
+ `(progn
+ (setq *tess-callbacks* '())
+ ,@(loop for (name callback-type args) in callback-specs
+ collect `(define-tessellation-callback ,name ,callback-type ,args))))
+
+(define-tessellation-callbacks
+ (tess-begin-data-cb :tess-begin-data (tessellator (type :unsigned-int) (polygon-data :pointer)))
@3b

3b Feb 23, 2011

Owner

I'd probably spell out -callback rather than -cb

@dardoria

dardoria Feb 24, 2011

Contributor

done

@3b 3b and 1 other commented on an outdated diff Feb 23, 2011

glu/interface.lisp
+
+(defmethod tess-begin-data-cb ((tess tessellator) which polygon-data)
+ (gl:begin which))
+
+(defmethod tess-error-data-cb ((tess tessellator) error-code polygon-data)
+ (error "Tessellation error: ~A~%" (error-string error-code)))
+
+(defmethod tess-end-data-cb ((tess tessellator) polygon-data)
+ (gl:end))
+
+(defun register-callbacks (tess)
+ (loop for tess-cb in *tess-callbacks*
+ when (compute-applicable-methods
+ (tess-callback-generic-function tess-cb)
+ (cons tess (loop repeat (tess-callback-arg-count tess-cb) collect t)))
+ do (progn
@3b

3b Feb 23, 2011

Owner

don't need PROGN with LOOP DO

@dardoria

dardoria Feb 24, 2011

Contributor

done

@3b 3b and 1 other commented on an outdated diff Feb 23, 2011

examples/redbook/tess-wind.lisp
+ (300 350 0) (300 150 0)
+ (150 150 0) (150 300 0)
+ (250 300 0) (250 200 0)
+ (200 200 0) (200 250 0)))
+ (quad1 '((50 150 0) (350 150 0)
+ (350 200 0) (50 200 0)))
+ (quad2 '((100 100 0) (300 100 0)
+ (300 350 0) (100 350 0)))
+ (tri '((200 50 0) (250 300 0)
+ (150 300 0))))
+
+ (glu:tess-property tobj :tess-winding-rule *current-winding*)
+
+ (gl:new-list *list* :compile)
+ (glu:tess-begin-polygon tobj nil)
+ (glu:tess-begin-contour tobj)
@3b

3b Feb 23, 2011

Owner

i'd use gl:with-new-list rather than gl:new-list + gl:end-list, and add similar macros for glu:tess-begin-polygon, glu:tess-begin-contour, and any similar functions

@dardoria

dardoria Feb 24, 2011

Contributor

Switched to gl:with-new-list. Will add the other macros.

Owner

3b commented Feb 23, 2011

Haven't looked at how it actually works closely yet, but made some notes of style issues while skimming it.

Biggest thing I see so far is exposing cffi pointers directly in the callbacks, I'd prefer to keep the main API using pure CL as much as possible, possibly splitting out a lower level API like the %GL package for people who want to work with the FFI pointers directly for performance.

dardoria added some commits Feb 24, 2011

@dardoria dardoria Apply comments from 3b:
- remove tess prefixes for enums
- use with-new-list
- rename cb to callback
- use enum for winding options instead of constants
310b66a
@dardoria dardoria Close parens. 1da03c4
Contributor

dardoria commented Feb 24, 2011

Thanks for your comments!

I applied most of your comments on style (I need to only add macros for begin polygon etc). A few comments on my approach:

  1. In the interface I tried to follow the model of glut:window.
  2. There are two flavors of callbacks: with user data and without. I've exposed only the ones with user data in the interface (although I don't see any real use of the user data)
  3. I agree with your point about exposing direct cffi data in the callbacks. However I had difficulties with memory management and thought I'll just leave it so for now. For instance the vertex-data in the vertex and combine callbacks can be of arbitrary length, so I need to keep track what was passed in tess-vertex in order to reconstruct it in the callbacks.
    Another issue is that in tess-vertex I cannot use with-foreign-pointer because the pointer to the vertex-data needs to be live until the polygon is constructed. The same goes for the vertex-data that the combine callback returns.
    I will try to come up with a solution for these...
  4. I have tested only on SBCL 1.0.36/linux
Contributor

dardoria commented Mar 12, 2011

Closing this for now. Will reopen when I have more time to finish.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment