GSR - FR (famrom@xxxxxxxxxxxxxxxxxxxx) wrote: > 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." Well, I should have sent my first two mails in a different order then perhaps. The second mail does exactly this: It discusses the presets. So I guess thats not the reason for you to be so offended. The first mail I sent discusses the way this IMHO should be implemented. In fact I did implement it in my local tree here yesterday and it turns out that it is a pretty big change, since I noted a lot of brokeness in the gimpdisplayshell API (the ratio 16:1 is encoded as both 1601 (decimal) and 0x1601 (hexadecimal) in different places of the code). Because of the size of my patch it might be too late to put this into 2.0. I think it is the right thing to do, but it might have to wait (the patch is attached to bug 131563). So in the retrospective the only reproach I have to make to myself is the choice of the word "evil" in IRC. I'd like to apologize for that - rest assured that I would not have used that word in a more formal medium like mailing lists (Ok, I used it after you brought it up). Please accept my apologies and I hope we can focus on a more technical discussion. [technical discussion :)] I think I already explained why I prefer the set of ratios based on the idea of "homogenous zooming". So the rest of this Mail focuses on the technical issues of your patch. I am not an expert on optimization levels, Assembler or whatever. The only thing I know is, that it is hard to compare two floats for equality. Part of the (huge) patch I mentioned above (http://bugzilla.gnome.org/showattachment.cgi?attach_id=23503) is a function, that calculates the next zoom step (float) based on a given (float) zoom step. It looks like this: gdouble gimp_display_shell_scale_zoom_step (GimpZoomType zoom_type, gdouble scale) { gint i, n_presets; gdouble new_scale; /* This table is constructed to have fractions, that approximate * sqrt(2)^k. This gives a smooth feeling regardless of the starting * zoom level. */ gdouble presets[] = { 1.0 / 256, 1.0 / 180, 1.0 / 128, 1.0 / 90, 1.0 / 64, 1.0 / 45, 1.0 / 32, 1.0 / 23, 1.0 / 16, 1.0 / 11, 1.0 / 8, 2.0 / 11, 1.0 / 4, 1.0 / 3, 1.0 / 2, 2.0 / 3, 1.0, 3.0 / 2, 2.0, 3.0, 4.0, 11.0 / 2, 8.0, 11.0, 16.0, 23.0, 32.0, 45.0, 64.0, 90.0, 128.0, 180.0, 256.0, }; n_presets = G_N_ELEMENTS (presets); /* Zooming in/out always jumps to a zoom step from the list above. * However, we try to guarantee a certain size of the step, to * avoid silly jumps from 101% to 100%. */ switch (zoom_type) { case GIMP_ZOOM_IN: scale *= 1.1; new_scale = presets[n_presets-1]; for (i = n_presets - 1; i >= 0 && presets[i] > scale; i--) new_scale = presets[i]; scale = new_scale; break; case GIMP_ZOOM_OUT: scale /= 1.1; new_scale = presets[0]; for (i = 0; i < n_presets && presets[i] < scale; i++) new_scale = presets[i]; scale = new_scale; break; case GIMP_ZOOM_TO: break; } return CLAMP (scale, 1.0/256.0, 256.0); } As you can see it compares floats with <= and >= and so avoids tests for real equalness. The little "jump" done by multiplying by 1.1 (which is a bit arbitrary chosen, but should be smaller than the factors between the presets) makes it even more robust IMHO. Something similiar could be implemented directly in gimp_display_shell_scale (). The only trick is, to get the numerator and denominator for the fraction in the gimpdisplayshell. In current CVS there is the function gimp_display_shell_scale_calc_fraction (), that calculates a fraction for a given float. It uses continued fractions, which kind of ensures, that things like 2.0 / 3.0 really result in 2/3. You could use this to determine the fraction for the gimpdisplayshell (similiar as to how it is used now). I hope you can accept this as a technical criticism of your patch, it might solve your floating point problems with a different approach. It also should work with a different set of presets. Thanks, Simon -- Simon.Budig@xxxxxxxxxxx http://www.home.unix-ag.org/simon/
