Pr 20210430 bugfixes

[0-8-15]

Various small changes accumulated over time.

[mgorges]

Thanks for trying to help us improve your codebase. I agree most of these are minor but you did include 'GLCORE: fix missing globalbinding and unbound variable' which is a pretty major rewrite of functionality? This one change also does a bunch of macro things, which I am worse at simply reading the code for to see what risks and problems might be?

[0-8-15]

Am Thu, 29 Apr 2021 11:53:27 -0700 schrieb Matthias Görges @.***>:

Thanks for trying to help us improve your codebase.

That's a bit early a compliment. The actual improvement is only in preparation stage so far. There is a glgui alike thing, which uses Feeley's concept of a closure compiler to speed up rendering.

Not too bad an effect. A desktop screenfull of text of ouf a 40MB Gambit generated C file renders at single digit load.

edited to add: more importantly it now takes about 30-60 seconds between garbage collections, not 2-4 per second as before.

I agree most of these are minor but you did include 'GLCORE: fix missing globalbinding and unbound variable' which is a pretty major rewrite of functionality? This one change also does a bunch of macro things, which I am worse at simply reading the code for to see what risks and problems might be?

Sorry, but the race condition the changes in #418 where only reliable reproducible for me once the faster rendering had the effect that more of the background i/o load was done on the GUI thread.

The whole macro magic has just one purpose: if compiled without debug if should expose exactly the same interfaces as before.

Wrt. improvements: no worries, I just took on a different job. Likely less time for lambdanative. But I'll submit that module when it is ready. Just now it's blocked on #418, because I don't want "double tap a button fast enough breaks application" assigned to a new GUI module while it is in fact a Gambit abuse.

[0-8-15]

One more review later: you are right: commit #145d9b5 should have been submitted in #418.

Absolutely.

It already references things introduced over there.

Sorry for that. You don't want the whole sequence of experimental commits to someone else git branch, do you?

I tried to break it into two pull requests. The one with the small things and the big one I really don't know how to break in piecemeal. I failed.

[mgorges]

Okay, so I have finally learned how to cherry-pick commits in pull requests with the Github Desktop app. I have added all commits except for 0603b04 and 145d9b5, and the cleanup from fb2a000 and 9c435d2, which were combined in c840068.

0603b04 I didn't merge because I believe MATURITY+1 is not defined (or I don't understand what it does) 145d9b5 I didn't merge as this is a combination of untested code, MATURITY+3 things, and macro magic I don't unterstand.

[0-8-15]

Am Wed, 26 May 2021 18:49:24 -0700 schrieb Matthias Görges @.***>:

@mgorges commented on this pull request.

@@ -169,6 +169,16 @@ ___result = GL_CLAMP_TO_EDGE;
((c-lambda (float float float) void "glTranslatef") (flo a) (flo b) (flo c)))

+(define glTranslatef//checks (c-lambda (float float float) void "glTranslatef"))

Can you explain what the // in variable names mean?

Following a sort-of tradition in naming convventions among some Scheme folks who pronounce e.g., hash-table-ref/default as "hash table ref WITH default" I began pronouncing a // as "without". So this would be the same a glTranslatef without those checks.

Looking at this from a distance in time, the name is actually slightly misleading, as the checks are still there (in the FFI generated code) while the calls to the converter functions which make sure the checks will never catch anything are removed.

[0-8-15]

Am Wed, 26 May 2021 18:51:04 -0700 schrieb Matthias Görges @.***>:

@mgorges commented on this pull request.

• ;;; -> fixnum)
• (define glCoreTextureCreate)
• ;; glCoreTextureReset -- TBD: unknown usage status
• ;;;
• ;;; (: glCoreTextureReset -> undefined)
• ;;;
• ;;; purpose: clear resources
• (define glCoreTextureReset)
• ;; Implementation (volatile)
• (define-type glCore:texture
• macros: prefix: %MATURITY+3%texture%macro-
• %%valid ;; FIXME: factor out from immutable components
• glidx ;; index (for opengl and internal table)
• %%-???-u32vector ;; what is this? mutable?

what are %%-??? functions - I don't understand the macro piece to adjudicate this so we can't merge it as I can't maintain it.

The idea behind this change is to texture representation from vectors (or where they even lists before?) into a distinct data type with accessors having names so I could understand the code better. (I'm not good in recalling the ordering in vectors, also greping for vector-ref with a certain index is above my head).

In doing so I had to invent names for the slots. As I did not understand what the u8data slot is actually good for, whether or not it is mutable etc. I gave it a strange name and left a comment behind.

By using gambit's facility to declare the accessors as macros (instead of procedures as they would become without the macros:... line) I made sure that they are not exported, do not pollute the toplevel namespace and are valid only within this module from the define-type clause downwards.

As it turns out there is only glCore:texture-data ever using it (other modules are now sure not accessing the texture behind the scenes anymore as they are no longer vectors and glCore:texture-data is only used three times. So the slot should actually be called just "data". (And as a consequence the global binding of glCore:texture-data should be removed for the better.

...

long story short: I made these changes ages ago at first in order to better understand the code and then MOST OF ALL because I found myself in the need to ensure that glCore:textures (the table) and glCore:tidx are no longer accidentally reachable outside of the procedures glCoreTextureReset, glCore:textures-ref and glCooreTexturesCreate.

Standard way to reach that goal: information hiding, pull them out of the toplevel, make them unreachable so things break obviously. That's the reason for the let around them.

...

Related: The whole block is within a cond-expand ;; CONSTRUCTION-CASE that's because the intention was to create code which would allow for optimizations (i.e. changes) -- initially without changing anything functional. Hence the else branch of the cond-expand still holds the old version.

If I had completed this change to the point where I am satisfied with the change, I had removed the cond-expand, digged out the %%-???-u32vector etc. before submitting. But than life changed, there was this race condition (other PR wrt. eventloop) killing the app which took me a few month to fix and the job change, which all put me into a hurry to submit it for this discussion to arise.

[0-8-15]

Am Wed, 26 May 2021 18:47:51 -0700 schrieb Matthias Görges @.***>:

@mgorges commented on this pull request.

@@ -298,10 +323,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
(loop (fl+ x0 gax) (cdr cs))))))

(define (glgui:fontheight fnt)

• (let* ((g (assoc 0 fnt))
• (i (if g (glgui:glyph-image g) #f))
• (h (if i (glgui:image-h i)
• (cadr (cadr (car fnt)))))) h))
• (cond
• ((macro-ln-ttf:font? fnt) (ttf:glyph-height (MATURITY+1:ln-ttf:font-ref fnt 0)))

What is MATURITY+1 here - it is not defined?

Ups. That's a mistake on my part.

The MATURITY prefix is a naming convention I'm using when changing code. The assumption behind is that there is a target level zero, which roughly means "stable&testes", negative values are things found to be problematic for one reason or another and positive values are the path forward but not yet well tested. Sure that assertion changes over time, while the names have to be manually dragged behind.

In order to not load the testing of my changes onto you, I'm working from my own branch and manually port things into a fresh branch from current master whenever I decide to prepare a PR. This ensures you get fewer PR's than me changing things. It comes at the cost that I have to manually port things over. For that I must use a different machine than my development envt. And that's bad, bad, bad.

In the case at hand it is so that I removed the MATURITY+1: from the actual definition but managed to not remove if from the call in glgui:fontheight.

Unfortunately my dev envt did have both definitions, though they are already the same. Hence I did not catch it.

[0-8-15]

Am Wed, 26 May 2021 18:52:26 -0700 schrieb Matthias Görges @.***>:

0603b04 I didn't merge because I believe MATURITY+1 is not defined (or I don't understand what it does) 145d9b5 I didn't merge as this is a combination of untested code, MATURITY+3 things, and macro magic I don't unterstand.

Well, in fact these MATURITY+3 things should be renamed already. They are ages old. Just that there are a few more things to be cleaned up the that module.

[0-8-15]

In the case at hand it is so that I removed the MATURITY+1: from the actual definition but managed to not remove if from the call in glgui:fontheight. Unfortunately my dev envt did have both definitions, though they are already the same. Hence I did not catch it.

Looking closer I found that this is not all it takes. I'll update. Both definitions are still in use in my development environment (and the integration/pull-request machine still can not build as it does not have enough resources).

Here is what is in my development environment:

(define (MATURITY+1:ln-ttf:font-ref font char) ;; -> glyph
  (cond
   ((macro-ln-ttf:font? font) ;; TBD: leave this as the only case
    (table-ref (macro-ln-ttf:font-char->glyph-table font) char #f))
   (else (error "illegal arguments" MATURITY+1:ln-ttf:font-ref font char))))

You see: this version returns then while entry as a distinct type while the backward compatible version returns the legacy representation. I'll try to get rid of the latter.

Update: I can not reproduce the error I made yesterday. The above definition is all it takes.

In the meantime I removed the MATURITY+1: prefix here and dropped the old definition. Still working.