Hi all,
First question first: Budeme psat do listu jen anglicky, nebo je cestina OK?
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
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?
****** While we are at it, we might unify GP_IS_CONTEXT_VALID and GP_CHECK_CONTEXT. Is the distinction intentional?
***** Please see GfxPrim dev wiki at http://atrey.karlin.mff.cuni.cz/~gavento/GfxPrimWiki/ (will be moved to gfxprim.ucw.cz) In particular http://atrey.karlin.mff.cuni.cz/~gavento/GfxPrimWiki/dev/naming with some sketches of naming scheme.
****** 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?
****** 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.
If we want to keep non-transforming functions public, then we have to provide access to GP_Context transformations (more accessors).
Tralala! Tomas
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.
Hi! Here comes some more notes for the API.
As it is now, most of the drawing functions returns GP_RetCode. It doesn't do any harm, however handling of the return value is inconsistent (especially for primitives that are draw using several other primitives) and generally the returned value is useless. So does anybody object to converting these to return void?
There are however some calls where GP_RetCode has it's meaning:
* GP_Text() - may return invalid string character (for encoding given by font) - now invalid characters are silently converted into spaces
* Functions that opens/saves files - should return if operation has failed
But all other drawing functions (like GP_Line(), GP_Circle(), etc...) should better return void.
On 04/24/2011 04:10 PM, Tomas Gavenciak wrote:
Hi all,
First question first: Budeme psat do listu jen anglicky, nebo je cestina OK?
Ahoj, ahoj,
Jsem pro anglictinu. Vsichni rozumime, vypada to vic l33t a navic, kdyby chtel nekdo ze zahranici opravit chybu, poslat patch nebo dokonce spolupracovat (clovek nikdy nevi), bude velmi vyhodne, kdyz si bude moci precist archiv konference.
Jo: prosim pana teto konference, aby nastavil 'Reply To' u mailu na gfxprim@ucw.cz; lepe se pak odpovida.
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
All three are OK with me; just a note: I personally use 'xmin' and 'xmax' (slightly shorter) but this is not much an issue.
The argument-modifying macros do not indicate which arguments are modified (even worse than C++ refs).
True, and I accept that it's ugly; we did not have any better idea.
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?
I think this is not a bad idea, but '_mod' is IMO misleading (can mean anything else, e.g. 'modulus'). Maybe 'x_out'?
While we are at it, we might unify GP_IS_CONTEXT_VALID and GP_CHECK_CONTEXT. Is the distinction intentional?
GP_IS_CONTEXT_VALID is just a test, GP_CHECK_CONTEXT is an assertion. Both are ugly and probably should not exist as they do now; when I wrote them, I tried to do as many safety checks as possible, like in Linux kernel, but I probably picked the wrong way to do it.
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?
I agree with the transformed functions being the default. However, "_Bare" is uncommon and IMO difficult to understand. I'm not even sure if the untransformed functions need to be public. The time cost of transformation is only an issue for very basic functions like putpixel; even for lines it should already be negligible.
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.
I would disagree here; two fields would, IMO, lead to confusion.
We need to somehow fix the context behavior anyway, and the freedom we will retain by using accessors will be probably very small.
Depends on how much 'intelligence' we want to put into the context later. When I think about the API, I feel more and more inclined to adding more status fields - like current color, etc. - to the Context to keep the number of arguments to drawing functions relatively small; otherwise, we will need to pass more and more various style-related settings through arguments. But that's just my opinion.
Best regards,
Jiri Dluhos
Hi!
Jo: prosim pana teto konference, aby nastavil 'Reply To' u mailu na gfxprim@ucw.cz; lepe se pak odpovida.
Should be fixed now.
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?
I agree with the transformed functions being the default. However, "_Bare" is uncommon and IMO difficult to understand. I'm not even sure if the untransformed functions need to be public. The time cost of transformation is only an issue for very basic functions like putpixel; even for lines it should already be negligible.
Well, I still could imagine circumstances where non transformed functions are good to have, but you may say that I have very good imagination...
The transformed functions are costy for putpixels and blits, not sure how much (it's probably the same O() just different constant).
We need to somehow fix the context behavior anyway, and the freedom we will retain by using accessors will be probably very small.
Depends on how much 'intelligence' we want to put into the context later. When I think about the API, I feel more and more inclined to adding more status fields - like current color, etc. - to the Context to keep the number of arguments to drawing functions relatively small; otherwise, we will need to pass more and more various style-related settings through arguments. But that's just my opinion.
When I use the API, I usually use like three or five colors at the time one for background, one for text and one or more for foreground. Following that, the context would need to contain Text Style and number of colors. Rather than that I prefer lightweight context to let the library user handle things himself.
Hi!
Well, I still could imagine circumstances where non transformed functions are good to have, but you may say that I have very good imagination...
Well we use some of these at least internally (GP_Fill() uses GP_FillRect()).
Hi,
I agree with the transformed functions being the default. However, "_Bare" is uncommon and IMO difficult to understand. I'm not even sure if the untransformed functions need to be public. The time cost of transformation is only an issue for very basic functions like putpixel; even for lines it should already be negligible.
Well, I still could imagine circumstances where non transformed functions are good to have, but you may say that I have very good imagination...
I would agree to keep them as "partially internal". Would you suggest a naming scheme?
The transformed functions are costy for putpixels and blits, not sure how much (it's probably the same O() just different constant).
I guess only for get/putpixel. Blit is very expensive, transformed or not.
We need to somehow fix the context behavior anyway, and the freedom we will retain by using accessors will be probably very small.
Depends on how much 'intelligence' we want to put into the context later. When I think about the API, I feel more and more inclined to adding more status fields - like current color, etc. - to the Context to keep the number of arguments to drawing functions relatively small; otherwise, we will need to pass more and more various style-related settings through arguments. But that's just my opinion.
When I use the API, I usually use like three or five colors at the time one for background, one for text and one or more for foreground. Following that, the context would need to contain Text Style and number of colors. Rather than that I prefer lightweight context to let the library user handle things himself.
I would also prefer to keep the core library as simple as possible, and have somewhat "modular" layout (context should be usable without text, images, events and backends).
If you really think that having a "drawing context" with bg/fg color, font and others would be useful, you can make wrappers of all the drawing functions accepting that parameter, such as: drawing_context=GP_DC_New(context, aliasing, font, fgcolor, bgcolor, ); GP_DC_Line(drawing_context, x1, y1, x2, y2, GP_FGCOLOR); but IMO the general usefulness would be limited.
I was thinking about adding "transformed w/h" to GP_Context to speed up GP_ContextW (avoid swap_axes checking), but that is probably not useful and can be added later.
Have a nice code, Tomas
Hi!
I agree with the transformed functions being the default. However, "_Bare" is uncommon and IMO difficult to understand. I'm not even sure if the untransformed functions need to be public. The time cost of transformation is only an issue for very basic functions like putpixel; even for lines it should already be negligible.
Well, I still could imagine circumstances where non transformed functions are good to have, but you may say that I have very good imagination...
I would agree to keep them as "partially internal". Would you suggest a naming scheme?
Hmm, I personally preffer something like GP_RLine() (for Raw Line).
We need to somehow fix the context behavior anyway, and the freedom we will retain by using accessors will be probably very small.
Depends on how much 'intelligence' we want to put into the context later. When I think about the API, I feel more and more inclined to adding more status fields - like current color, etc. - to the Context to keep the number of arguments to drawing functions relatively small; otherwise, we will need to pass more and more various style-related settings through arguments. But that's just my opinion.
When I use the API, I usually use like three or five colors at the time one for background, one for text and one or more for foreground. Following that, the context would need to contain Text Style and number of colors. Rather than that I prefer lightweight context to let the library user handle things himself.
I would also prefer to keep the core library as simple as possible, and have somewhat "modular" layout (context should be usable without text, images, events and backends).
True indeed. I was thinking of this too, we IMHO have too much functionality in core now. At least I would move the text related code to it's own directory.
If you really think that having a "drawing context" with bg/fg color, font and others would be useful, you can make wrappers of all the drawing functions accepting that parameter, such as: drawing_context=GP_DC_New(context, aliasing, font, fgcolor, bgcolor, ); GP_DC_Line(drawing_context, x1, y1, x2, y2, GP_FGCOLOR); but IMO the general usefulness would be limited.
Yes modular could easily apply here.
I was thinking about adding "transformed w/h" to GP_Context to speed up GP_ContextW (avoid swap_axes checking), but that is probably not useful and can be added later.
I understand that, but there are at least two more problems with that:
* having two w/h is confusing * with two w/h context could be in inconsistent state - you must add number of checks into the code
In short, I don't think this is worth of the work.
Hi,
Well, I still could imagine circumstances where non transformed functions are good to have, but you may say that I have very good imagination...
I would agree to keep them as "partially internal". Would you suggest a naming scheme?
Hmm, I personally preffer something like GP_RLine() (for Raw Line).
Now there is a good flame! Lets feed it a little:
I disagree with RLine - HLine is the name "Horizontal line", but raw/nonraw is orthogonal to that. I would hate to have [ridiculous example] "ATRHLine" for antialiased, translucent, raw and horizontal line. Compare with AA_HLine_RAW, HLine_AA_Raw or HLine_AR (as one-letter flags, but still prune to collisions)
While having flags as non-separated prefixes seems tempting, it will lead to ambiguous names, order confusion etc.
In general, it is a good question which separate variants to generate for combinations of Antialiasing, Transparency and Raw(nontransformed).
Another general point is that we might want to split drawing functions off core to a different "module", keeping putpixel and perhaps writepixels.
I would also prefer to keep the core library as simple as possible, and have somewhat "modular" layout (context should be usable without text, images, events and backends).
True indeed. I was thinking of this too, we IMHO have too much functionality in core now. At least I would move the text related code to it's own directory.
Agree.
I was thinking about adding "transformed w/h" to GP_Context to speed up GP_ContextW (avoid swap_axes checking), but that is probably not useful and can be added later.
I understand that, but there are at least two more problems with that:
- having two w/h is confusing
- with two w/h context could be in inconsistent state
- you must add number of checks into the code
I understand that having two public attributes would be confusing, but if we really want to keep it "secret", then I do not see the confusion problem.
How would you get inconsistency in GP_Context? Context and its attributes are once set-up on context creation and then never modified again (only clipping may be modified afterwards).
Inconsistency can either be introduced upon creation - either in core lib (we can have a debug check tests there) or manual initialization (should be forbidden); or by modification afterwards - either manually (if anybody modifies these, no CHECKs can save her from doom) or by memory corruption (then the CHECK is useless anyway and not much more informative that SEGV).
It is a pity that C does not have const struct members as C++ classes do.
T.
Hi!
Well, I still could imagine circumstances where non transformed functions are good to have, but you may say that I have very good imagination...
I would agree to keep them as "partially internal". Would you suggest a naming scheme?
Hmm, I personally preffer something like GP_RLine() (for Raw Line).
Now there is a good flame! Lets feed it a little:
I disagree with RLine - HLine is the name "Horizontal line", but raw/nonraw is orthogonal to that. I would hate to have [ridiculous example] "ATRHLine" for antialiased, translucent, raw and horizontal line. Compare with AA_HLine_RAW, HLine_AA_Raw or HLine_AR (as one-letter flags, but still prune to collisions)
Okay, you have a good point here. The _RAW suffix sounds good to me.
In general, it is a good question which separate variants to generate for combinations of Antialiasing, Transparency and Raw(nontransformed).
Having {normal, AA, software transparency} x {_RAW, normal} makes 6 differnt variants...
It's quite a number, but still I think that it makes sense.
Another general point is that we might want to split drawing functions off core to a different "module", keeping putpixel and perhaps writepixels.
Hmm, I think that having drawing in core makes sence. I proposed moving the text part out of it, because there is text loader/saver and there is possibility that the number of feauters would grow => it should not live in core.
Maybe something like "If it utilizes file format loader" it's not a core. Or more like, only basic drawing primitives and blits in core.
I was thinking about adding "transformed w/h" to GP_Context to speed up GP_ContextW (avoid swap_axes checking), but that is probably not useful and can be added later.
I understand that, but there are at least two more problems with that:
- having two w/h is confusing
- with two w/h context could be in inconsistent state
- you must add number of checks into the code
I understand that having two public attributes would be confusing, but if we really want to keep it "secret", then I do not see the confusion problem.
Well, that is not easy done in C. Either the whole struct is public or not. And if it's not, you can't create it statically...
How would you get inconsistency in GP_Context? Context and its attributes are once set-up on context creation and then never modified again (only clipping may be modified afterwards).
Inconsistency can either be introduced upon creation - either in core lib (we can have a debug check tests there) or manual initialization (should be forbidden); or by modification afterwards - either manually (if anybody modifies these, no CHECKs can save her from doom) or by memory corruption (then the CHECK is useless anyway and not much more informative that SEGV).
It is a pity that C does not have const struct members as C++ classes do.
Hmm, you could create const int real_w; member and change it from library code as:
int *real_w = (int*) &context->real_w;
*real_w = context->w;
Hi!
How would you get inconsistency in GP_Context? Context and its attributes are once set-up on context creation and then never modified again (only clipping may be modified afterwards).
That is not true, context orientation flags (axes_swap, mirror_x and mirro_y) could be modified any time. So the actually inconsistency may arise from changing axes_swap.
Furthermore as it is now, the orientation flags are inicialized to 0 at the time of the context creation and may only be changed later.
Hi,
How would you get inconsistency in GP_Context? Context and its attributes are once set-up on context creation and then never modified again (only clipping may be modified afterwards).
That is not true, context orientation flags (axes_swap, mirror_x and mirro_y) could be modified any time. So the actually inconsistency may arise from changing axes_swap.
Maybe I did not point that out enough:
-- Tomas wrote: -- Inconsistency can either be introduced upon creation - either in core lib (we can have a debug check tests there) or manual initialization (should be forbidden); or by modification afterwards - either manually (if anybody modifies these, no CHECKs can save her from doom) or by memory corruption (then the CHECK is useless anyway and not much more informative that SEGV).
While the values CAN be modified by the user (not being const), they MUST NOT. If anybody does that, let them eat steaming *(NULL).
-- Cyril wrote: -- Furthermore as it is now, the orientation flags are inicialized to 0 at the time of the context creation and may only be changed later.
This may be a *temporary* solution, but this must be done by some setup function sooner rather than later. We need to know swap_axes for bytes_per_row in allocation anyway. Requiring user to do this manually and then force her to use GP_ContextWidth is insanity.
Either we declare GP_Context entirely public (declare it read-only xcept for clipping, then drop most wrappers, do sanity checks and keep our fingers crossed) or go with wrappers for everything ESPECIALLY such dangerous internal stuff (such as bytes_per_line and h) and consistently use these wrappers ourselves (if we want to have any chance to be able to modify GP_Context).
We can do either but definitely not both.
Tomas
Hi!
Furthermore as it is now, the orientation flags are inicialized to 0 at the time of the context creation and may only be changed later.
This may be a *temporary* solution, but this must be done by some setup function sooner rather than later. We need to know swap_axes for bytes_per_row in allocation anyway. Requiring user to do this manually and then force her to use GP_ContextWidth is insanity.
Maybe you don't get that right. The default settings are 0, 0, 0. And then user is encouraged to change these at runtime (that is the functionality these flags are introduced for).
Hint: Think of the rotation flags as you are thinking of cliping, they may change at any time.
There are some wrappers ready that rotates these flags clock wise and counter clock wise. But as it is now, changing flag value is all that is needed to change drawing direction. I'm not against doing more wrappers and declaring that changing these directly is forbiden. But then, either you have wrappers for manipulating rotation flags or for getting width and height. Also I agree that having wrappers for W and H is more annoying that wrappers for context orientation.
And no, swap_axes only changes logical orientation of the bitmap, no need to know these in time context is allocated. The context is always allocated in orientation 0, 0, 0 with physical w and h.
Either we declare GP_Context entirely public (declare it read-only xcept for clipping, then drop most wrappers, do sanity checks and keep our fingers crossed) or go with wrappers for everything ESPECIALLY such dangerous internal stuff (such as bytes_per_line and h) and consistently use these wrappers ourselves (if we want to have any chance to be able to modify GP_Context).
The problem with w and h, and moreover the reason why we have GP_TContextW() and GP_TContextH() is that logical w and h depends on axes_swap, while physical stays same from the point context was initialized.
Still I'm not sure, what is ideal solution here. Maybe wrappers for operations that are not usual and direct access to these that are used often.