Hi!
First question first: Budeme psat do listu jen anglicky, nebo je cestina OK?
Ja bych preferoval anglictinu, stejne ji vsichni umime....
Before going to the individual ideas, I would like to ask you for your opinions and for help with refactoring the current code (including names), thinking about the people that will use the library, frequent use cases and "most-expected" behaviour.
I would like to propose few renames in GfxPrim, mostly to improve readability:
GP_Context: x_swap, y_swap -> flip_x, flip_y It is called "vertical/horizontal flip", meaningful-sentence-like identifiers are easier to remember and read (flip_x vs x_flip) GP_Context: axes_swap -> swap_axes Readability, consistency with flip_* GP_Context: clip_(w|h)_(min|max) -> clip_x_min etc. clipping coordinate, not width
Fine with me.
The argument-modifying macros do not indicate which arguments are modified (even worse than C++ refs). I propose to at least consistently document this in macro defs and/or change the argument names from "x" to "x_mod" or similar (i.e. in GP_Transform.h". Is there a LKML convention for that?
Well, this IMHO falls under "C is a spartan language" section. There is IMHO no better way to do this (and renaming the parameters wouldn't help here, everybody who opens the header file could clearly see what is done there).
So only proposal from me for this is to write good documentation.
While we are at it, we might unify GP_IS_CONTEXT_VALID and GP_CHECK_CONTEXT. Is the distinction intentional?
There is GP_IS_CONTEXT_VALID() returns integer value, while GP_CHECK_CONTENT() uses GP_CHECK() which calls abort.
The GP_IS_CONTEXT_VALID() was IMHO ripped from the code in order to avoid duplications. Perhaps static inline function would be better here.
And yes, the checks are written in inconsistent manner and would surely need unification.
It definitely makes sense to have transforming+clipping versions of operations as well as "bare" unsafe variants. I would propose to remove the "T" prefix from transforming variants, as most users will want the transforming behavior and to modify the names of the "bare" functions. I would propose "GP_Line_" or "GP_Line_Bare". What do you think?
Having the transformed operations as default sounds reasonable to me.
On the other hand I dislike indentifiers ending with '_'. What about something shorter like GP_LineRaw() GP_LineB() GP_LineR()... ?
Next proposal/question is about declaring public vs. private APIs. Most of it is public by default, but then we have accessors for GP_Context and others. What is the goal there?
Do we want GP_Context to be (partially) public, or do we want to provide accessors for everything? I would propose to make it public (to simplify things) and to solve transformation problems by having different fields for "user" w/h and "real" w/h. We need to somehow fix the context behavior anyway, and the freedom we will retain by using accessors will be probably very small.
Generally I preffer accesors as they allow to change internal library structure without breaking the API. So as I see it the Context is kind of semi public inteface => You could play with it if you need to, but when it breaks, you are on your own.
And I don't like the idea of several w and h. It would be IMHO more confusing than it is now.
If we want to keep non-transforming functions public, then we have to provide access to GP_Context transformations (more accessors).
I don't see a problem with that.