Simon.Budig@xxxxxxxxxxx (2004-01-17 at 1724.31 +0100):
> a) It has a table of presets that is constructed in an ad hoc manner
> without any explantation what the advantages to the user are.
Did your copy of the first mail I posted lacked:
"Comments about the presets also welcomed, I just made a list of the
ones that seemed interesting while working always around some given
factor."
Maybe I should have said more clearly:
"Test values, just to get the ball rolling."
But I am sure it is hard to confuse with:
"These are the tried and true values thou shalt use."
> b) according to yourself the behaviour of your patch changes depending
> on the existance of a certain printf.
It seems I found a compiler / arch weird sceneario about floats. But
from what I see, I was just unlucky, cos Gimp code has things like
mine too, I just stressed the tests (press zoom keys for 5 secs after
you are at the max was one of such tests) and it broke.
I have been checking the following docs, just trying to figure what it
could be (first time I find a problem like this, and nobody I asked
figured):
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12331
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=323
http://gcc.gnu.org/bugs.html#nonbugs_general
http://www.validlab.com/goldberg/paper.ps
And I have tried the following, and only the last worked:
-O2
-O2 -ffloat-store
-O2 -fno-strict-aliasing
-O2 -ffloat-store -fno-strict-aliasing
-O0
I have tried using gdoubles, gfloats or gldoubles (which failed about
not defined even after including gtypes.h, so tried long double, but
failed anyway), and declare all volatile except the array which I
declared const, and still the only way is -O0.
Later Chris Want started reviewing the code, and we decided to put ()
and CLAMPing everywhere we had doubts, then it works. Only thing I can
guess is that the opimization + fast repeat means rounding problems,
and other methods that made the floats "behave" (slow or printf, ie),
hide the problem.
> c) Again in your own words apparently one version of your patch fails
> in certain zoom steps (this is taken from
> http://www.infernal-iceberg.com/gimp/gimpdisplayshell-scale.c.diff
> which right now reads:
I will scrap "release early, release often" and "submit so a disk
crash does not mean lost work" from the list of wise phrases then.
I guess it would have been pretty nice to become mad alone instead of
get people trying to help me and point me to valgrind, optimization
levels, objdump and whatever they could think as useful.
[...]
> Sorry, I believe that a patch that depends on the compilers optimization
> level has indeed a real problem - for me this is evil.
I just read the C code, and it "mentally" compiles. But the compiler
does tricks and it fails. I am not an expert at this, I do not know C
ANSI spec, GCC design or x86 blue prints like if I had wrote them
myself, so I try my best.
Getting different values cos the compiler decides you can go with
different data size depending if it keeps all or not in CPU is not
fun, not at all. If you want to take the position of "not a bug",
fine, I prefer the position of "float is float, not maybe a double or
maybe a long double", specially cos if I want to get dirty, I should
be using asm directly, not letting the compiler throw mud at me. I
thought rounding was a tricky thing, now I know it is more than just
tricky, it has a random factor.
> On a more social level you failed to explain what problem you wanted to
> solve with this patch in IRC. I thought hard about my proposal and I
> indeed think that I can justify why I am choosing this set of presets,
> although I still would prefer the fully homogenous version, but I can
> see that people have issues with weird zoom percentages.
Proof of concept to get 1:n ratios to sweeten the interpolation and
desire to avoid getting a "submit a patch" when requesting something,
as I have got some (too many?) times in the past. Instead it seems
that when I show patch I get a "discard your patch and talk about the
request".
> > So I only have a question: why is homougenous zooming the holy grail
> > that makes the rest of issues discardable? Something other than the
> > words smooth or continous, which only make me think about animation
> > and not about painting.
> The whole point is user experience. "Homogenous Zooming" - for a lack
> of a better word - would behave exactly the same in every zoom level,
> regardless if this is from 274% or from 300%. When zooming in always
> the same rectangular area would be magnified to the image window.
Until both input and output is nicely interpolated, 274% will be less
predicatable than 300%, it is less pleasing when you look and when you
try to paint. Until I started this, Gimp was going with sqrt and no
smooth interpolation, so I tried to start scratching my itch, share
the results back and get suggestions. It seems I just managed to open
hell's gates instead.
> The user would be able to estimate, how often he has to zoom in to
> get *this* detail at *that* size.
I do not estimate, I zoom and stop when it looks suitable. Personally
I have keys assigned to the power of 2 zooms (stops at 16, but up to
present day I always worked with images that suit those limits), and
do a quick fly over them, to end adjusting with single steps if I am
undecided between two of the power based keys. Or use a rectangle
select, followed by a single readjust to reduce the jaggies, if I need
to fit something.
[...]
> In my earlier mail I presented a possible compromise between
> homogenous zooming (I hope I made the advantages of that approach clear)
> and 1:x based zooming. I think it keeps best of both worlds and would
> like to ask, that alternative proposals have a careful discussion of
> advantages vs. backdraws against this.
Minus the 2:45 and 2:3 ones, it matches a perfect 1:n, like one of my
suggestions. I pushed 2:45 to 1:22 and updated my patch on my site
(seems I have not managed to get rid of those two phrases yet). Also
attached.
http://www.infernal-iceberg.com/gimp/gimpdisplayshell-scale.c-floatfixed.diff
GSR
Index: app/display/gimpdisplayshell-scale.c
===================================================================
RCS file: /cvs/gnome/gimp/app/display/gimpdisplayshell-scale.c,v
retrieving revision 1.66
diff -u -p -r1.66 gimpdisplayshell-scale.c
--- app/display/gimpdisplayshell-scale.c 11 Jan 2004 12:55:45 -0000 1.66
+++ app/display/gimpdisplayshell-scale.c 18 Jan 2004 18:18:48 -0000
@@ -72,32 +72,124 @@ gimp_display_shell_scale_zoom_fraction (
gint *scalesrc,
gint *scaledest)
{
- gdouble ratio;
+ /* Presets array stores the zoom by 2, to be able to have 1.5 */
+ /* The sequence is an approximation to N+1 = N*sqrt(2) */
+#define NUM_PRESETS (16)
+ const gint presets[NUM_PRESETS] = {
+ 2, 3, 4, 6, 8, 12, 16, 22, 32, 44, 64, 90, 180, 256, 360, 510
+ };
+ gint src, dest;
+ gint nratio;
+ gfloat ratio;
+ gboolean swapped = FALSE;
+ gint shift, mid;
+ gint low = 0;
+ gint high = (NUM_PRESETS-1);
g_return_if_fail (scalesrc != NULL);
g_return_if_fail (scaledest != NULL);
- ratio = (double) *scaledest/ *scalesrc;
+ src = *scalesrc;
+ dest = *scaledest;
- switch (zoom_type)
+ if (! ((zoom_type == GIMP_ZOOM_IN) || (zoom_type == GIMP_ZOOM_OUT)))
{
- case GIMP_ZOOM_IN:
- ratio *= G_SQRT2;
- break;
-
- case GIMP_ZOOM_OUT:
- ratio /= G_SQRT2;
- break;
-
- default:
- *scalesrc = CLAMP (zoom_type % 100, 1, 0xFF);
- *scaledest = CLAMP (zoom_type / 100, 1, 0xFF);
- return;
- break;
+ /* zoom_type is an "encoding" of desired zoom */
+ src = CLAMP (zoom_type % 100, 1, 0xFF);
+ dest = CLAMP (zoom_type / 100, 1, 0xFF);
+ }
+ else if ((src == 1) && (dest == 1))
+ {
+ /* Flip point, manual handling, the table is unable to handle this */
+ switch (zoom_type)
+ {
+ case GIMP_ZOOM_IN:
+ /* 1:1 -> 3:2 */
+ dest = 3;
+ src = 2;
+ break;
+
+ case GIMP_ZOOM_OUT:
+ /* 1:1 -> 2:3 */
+ dest = 2;
+ src = 3;
+ break;
+ }
+ }
+ else
+ {
+ /* Handling by presets, requires scaling ratio by 2 to fit table */
+ ratio = ((gfloat) dest) / ((gfloat) src);
+ if (ratio < 1.0)
+ {
+ swapped = TRUE;
+ ratio = (2.0 / ratio);
+ }
+ else
+ {
+ ratio *= 2.0;
+ }
+
+ /*
+ Readjust into N:2 (or 2:N) erring in the right side so
+ direction is kept and jump is only one step
+ swapped FALSE: 2 3 4 6 8
+ IN -> floor
+ OUT <- ceil
+ swapped TRUE: 8 6 4 3 2
+ IN -> ceil
+ OUT <- floor
+ */
+ if (((zoom_type == GIMP_ZOOM_IN) && (swapped == TRUE)) ||
+ ((zoom_type == GIMP_ZOOM_OUT) && (swapped == FALSE)))
+ {
+ nratio = ceil(ratio);
+ shift = -1;
+ }
+ else
+ {
+ nratio = floor(ratio);
+ shift = 1;
+ }
+
+ /* Binary search, get hit or nearest position */
+ while (low <= high) {
+ mid = (low + high) / 2;
+
+ if (presets[mid] == nratio)
+ break;
+ if (presets[mid] > nratio)
+ high = mid - 1;
+ else
+ low = mid + 1;
+ }
+
+ /* Unscale, making sure all values are in range */
+ mid += shift;
+ mid = CLAMP (mid, 0, (NUM_PRESETS-1));
+ dest = presets[mid];
+ if ((dest % 2) == 0)
+ {
+ src = 1;
+ dest = CLAMP ((dest/2), 1, 0xFF);
+ }
+ else
+ {
+ src = 2;
+ dest = CLAMP (dest, 1, 0xFF);
+ }
}
- /* set scalesrc and scaledest to a fraction close to ratio */
- gimp_display_shell_scale_calc_fraction (ratio, scalesrc, scaledest);
+ if (swapped)
+ {
+ *scalesrc = dest;
+ *scaledest = src;
+ }
+ else
+ {
+ *scalesrc = src;
+ *scaledest = dest;
+ }
}
void
