Subject: Re: DGradient - One mo' bug

From: Dominik Vogt <[email-masked]>
Date: Mon, 1 Feb 1999 11:14:18 +0100

On Sun, Jan 31, 1999 at 10:18:39PM -0500, Bob Woodside wrote:
> 	This is what I did:
> 
> --- menus.c	Sun Jan 31 21:44:00 1999
> +++ menus-patched.c	Sun Jan 31 21:43:35 1999
> @@ -2098,8 +2098,9 @@
>  	      if( i % dc == 0 )
>  		XSetForeground(dpy, Scr.TransMaskGC,
>  			       ms->look.face.u.grad.pixels[cindex++]);
> -	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, (int)(i*ds), mr->height,
> -			mr->width, i);
> +/*  RBW  --  Test - try arbitrarily subtracting 1 from x values... */
> +	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, ((int)(i*ds) - 1), mr->height,
> +			(mr->width - 1), i);                            ^^^^^^^^^^
                        ^^^^^^^^^^^^^^^ <-- OK                          WRONG
These ones seem to be bugs. If the menu is n times m
pixels wide/high, the biggest valid x/y values are n-1 and
m-1. I.e. x = mr->width and y = mr->height are not inside the
menu. Wasn't there some discussions recently if the end points
of a line are drawn (XDrawLine)? The same applies to other
parts of the diagonal gradient code.

>  	    }
>  	  }
>          }

If this solves the problem in this situation there must be
a bug in the compiler or some math library or your X server.
Anyway, I wouldn't trust a linewidth of one painting every
pixel. Jay, did you have a look at how it's done e.g. for
title bars?

I dont understand these lines:

  if( i % dc == 0 )
    XSetForeground(dpy, Scr.TransMaskGC,
                   ms->look.face.u.grad.pixels[cindex++]);

What are the circumstances under which the cindex is increased
(in human readable language)?

Bye

Dominik

-- 
Dominik Vogt, Hewlett-Packard GmbH, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/14-4596, fax: 07031/14-3883, [email-masked]
--
Visit the official FVWM web page at <URL:http://fvwm.math.uh.edu/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: Tim Phipps <[email-masked]>
Date: Mon, 01 Feb 1999 16:27:20 +0000

Bob Woodside wrote:
> +                       (mr->width - 1), i);

It looks like you have a server bug, at least that would be my guess at
why you get the vertical lines left behind.  The lines are drawn in
mr->w and they shouldn't go outside.  The missing pixel lines are where
the code goes from drawing the top left corner to the bottom right. It
could be that rounding errors are getting in and by moving the lines
left by one pixel you fix them.  I don't get the missing pixels so this
fix makes the centre line be drawn twice for me.  There should be
missing lines in BGradients for you and they should be fixed the same
way.

To be honest I don't like the look of D/BGradients, I'd rather see the
lines drawn at 45degrees. Here's a patch that implements this and does
not use any floating arithmetic.  It's less code since it does the
gradient in one stage rather than two and it should not have any missing
pixels, but it does draw lines to points outside the window (the server
should clip them).  Each line is at 45 degrees and only one pixel from
its neighbour. To me this looks better when you have visible bands in
the gradient (DGradient 4 blue white). Before submenu gradients would be
at different angles to their parents.

If this fixes your problems we should vote on whether it goes in (I
abstain, but then I don't get missing pixels).

Cheers,
Tim.

Index: menus.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/menus.c,v
retrieving revision 1.41
diff -u -B -b -r1.41 menus.c
--- menus.c	1999/01/29 09:16:52	1.41
+++ menus.c	1999/02/01 16:17:49
@@ -2029,7 +2029,7 @@
 	  }
 	  XClearWindow(dpy, mr->w);
         }
-        else if ( type == VGradMenu )
+        else if (type == VGradMenu)
         {
 	  if (mr->backgroundset == False)
 	  {
@@ -2055,102 +2055,28 @@
 	  }
 	  XClearWindow(dpy, mr->w);
         }
-        else if ( type == DGradMenu )
+        else /* D or BGradient */
         {
-	  register int i = 0, dc;
-	  float ds;
+	  register int i = 0, numLines;
 	  int cindex = 0;
 
 	  XSetClipMask(dpy, Scr.TransMaskGC, None);
-	  if( mr->width > mr->height )
-          {
-	    ds = (float) mr->height / mr->width;
-	    dc = mr->width * 2 / ms->look.face.u.grad.npixels + 1;
-	    for(i = 0; i < mr->width; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, 0, (int)(i*ds), i, 0);
-	    }
-	    for(i = 0; i < mr->width; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, i, mr->height, mr->width,
-			(int)(i*ds));
-	    }
-	  }
-	  else
-	  {
-	    ds = (float) mr->width / mr->height;
-	    dc = mr->height * 2 / ms->look.face.u.grad.npixels + 1;
-	    for(i = 0; i < mr->height; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, 0, i, (int)(i*ds), 0);
-	    }
-	    for(i = 0; i < mr->height; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, (int)(i*ds), mr->height,
-			mr->width, i);
-	    }
-	  }
-        }
-        else
-        {
-	  register int i = 0, dc;
-	  float ds;
-	  int cindex = 0;
-
-	  XSetClipMask(dpy, Scr.TransMaskGC, None);
-	  if( mr->width > mr->height )
-          {
-	    ds = (float) mr->height / mr->width;
-	    dc = mr->width * 2 / ms->look.face.u.grad.npixels + 1;
-	    for(i = 0; i < mr->width; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, 0, mr->height - (int)(i*ds), i,
-			mr->height);
-	    }
-	    for(i = 0; i < mr->width; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, i, 0, mr->width,
-			mr->height - (int)(i*ds));
-	    }
-	  }
+          XSetForeground(dpy, Scr.TransMaskGC, ms->look.face.u.grad.pixels[0]);
+          numLines = mr->width + mr->height - 1;
+          for(i = 0; i < numLines; i++)
+          {
+            if((int)(i * ms->look.face.u.grad.npixels / numLines) > cindex)
+            {
+              /* pick the next colour (skip if necc.) */
+              cindex = i * ms->look.face.u.grad.npixels / numLines;
+              XSetForeground(dpy, Scr.TransMaskGC, ms->look.face.u.grad.pixels[cindex]);
+            }
+            if (type == DGradMenu)
+              XDrawLine(dpy, mr->w, Scr.TransMaskGC,
+	                0, i, i, 0);
 	  else
-          {
-	    ds = (float) mr->width / mr->height;
-	    dc = mr->height * 2 / ms->look.face.u.grad.npixels + 1;
-	    for(i = 0; i < mr->height; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, 0, mr->height - i, (int)(i*ds),
-			mr->height);
-	    }
-	    for(i = 0; i < mr->height; i++)
-	    {
-	      if( i % dc == 0 )
-		XSetForeground(dpy, Scr.TransMaskGC,
-			       ms->look.face.u.grad.pixels[cindex++]);
-	      XDrawLine(dpy, mr->w, Scr.TransMaskGC, (int)(i*ds), 0, mr->width,
-			mr->height - i);
-	    }
+	      XDrawLine(dpy, mr->w, Scr.TransMaskGC,
+	                0, mr->height - 1 - i, i, mr->height - 1);
 	  }
         }
         break;

---

From: Bob Woodside <[email-masked]>
Date: Mon, 1 Feb 1999 13:44:52 -0500 (EST)

> abstain, but then I don't get missing pixels).

	This did fix the problem. Also the gradients seem to look
smoother. Based on this, I'll cast a blind vote (meaning I haven't
taken the time to look at the code, I just applied your patch and tried
it) for including this. And maybe we can get 2.1.11 out Real Soon Now!

- Bob

---

From: Jay <[email-masked]>
Date: Mon, 1 Feb 1999 23:43:38 -0500 (EST)

> pixel. Jay, did you have a look at how it's done e.g. for

	I did just now.. the gradients on buttons/titlebars are drawn
using XFillRectangle.  There is, however, a XSetClipMask in there (at
least if PIXMAP_BUTTONS is defined.  If there was any 'accidental'
bleedover it would clip it.  Perhaps this is something we should try for
the D/Bgradients.  Though if it is buggy XServer code, there probably
isn't much we can do.  

> (in human readable language)?

	This line appears just before the for loop:

	dc = mr->width * 2 / ms->look.face.u.grad.npixels + 1;

	What this does is set dc to the number of pixels in each band of
color for the given gradient.  It bases its estimate on a window twice the
width of the menu it is drawing. 

	These lines, then:

	if( i % dc == 0 )
  
	Test to see if i is evenly divisible by the color-band width.  If
it is, then it's time to switch to the next color in the gradient.  

I am beginning to think we should either trash D/BGradients or confine
them somehow.. Perhaps doing something like WindowMaker and making the
D/Bgradients localized... IE- in a small box, say the size of a single
menu entry.  Maybe the 45 degree-version would do well with this regard, I
have not looked at it yet..  

or maybe just fix them as best we can and have a disclaimer: 'Processor
intensive and possibly ugly' ;P

Jay