vector symbol size calculation is incorrect

[tbonfort]

in vector symbols, if any of the coordinates contain negative values, or if min(x) or min(y) is not exactly 0, then the size calculations and the positionning of the symbol is incorrect.

people used these kind of point coordinates to be able to offset the rendering of vector symbols, but this kind of usage is now unnecessary as we have ANCHORPOINT support

I suggest that we now translate the input vector points to ensure they are all positive and that min(x) = min(y) = 0

[tbonfort]

fixed in r12873

[havatv]

Sorry, but this was changing behaviour a bit too much for me.

I agree that we should not allow negative values anymore, and shifting symbols to avoid negative coordinate values is acceptable for all cases, as far as I can see.

ANCHORPOINT allows very flexible placement for point symbols, so for point symbols, the suggested solution is OK. However, the situation is different for polygon fills.

In order to create polygon fill symbols in a flexible way, it is extremely useful that it is possible to specify the left and "bottom" part of the symbol to be "blank" (see a simple example in the figure below). If the symbol coordinates are shifted down to (0,0), this possibility will be lost.

-------------------
|        XX     XX|
|         XX   XX |
|          XX XX  |
|           XXX   |
|                 |
|                 |
-------------------

The "lower" corner of the bounding box of the points used to specify a vector symbol must be allowed to have x and y values greater than 0.

Maintaining the desired behaviour for polygon fills could be achieved by changing:

 if(minx!=0 || miny!=0) { 
    for(i=0;i<s->numpoints;i++) { 
       if(s->points[i].x != -99 && s->points[i].y != -99) { 
          s->points[i].x -= minx; 
          s->points[i].y -= miny; 
       } 
    } 
    s->sizex -= minx; 
    s->sizey -= miny; 
 } 

to:

 if(minx<0) { 
    for(i=0;i<s->numpoints;i++) { 
       if(s->points[i].x != -99 && s->points[i].y != -99) { 
          s->points[i].x -= minx; 
       } 
    } 
    s->sizex -= minx; 
 } 

 if(miny<0) { 
    for(i=0;i<s->numpoints;i++) { 
       if(s->points[i].x != -99 && s->points[i].y != -99) { 
          s->points[i].y -= miny; 
       } 
    } 
    s->sizey -= miny; 
 } 

or (to save a for loop):

 if(minx<0 || miny<0) { 
    for(i=0;i<s->numpoints;i++) { 
       if(s->points[i].x != -99 && s->points[i].y != -99) { 
          if(minx<0) { 
             s->points[i].x -= minx; 
          }
          if(miny<0) { 
             s->points[i].y -= miny; 
          }
       } 
    } 
    if(minx<0) { 
      s->sizex -= minx; 
    }
    if(miny<0) { 
      s->sizey -= miny; 
    }
 }

I reopen the ticket.

[tbonfort]

Maybe the translation can be done unconditionally if the symbol has an anchorpoint set to a non default value, and done as you propose if not? (as anchorpoint is not used for polygon fills, and this seems to be the only use-case where it might be desirable not to have the symbol start at (0,0) )

[havatv]

May be. I have no strong feelings about that. The most important thing is to keep the polygon fill behaviour.

I think we should strive to make things as consistent (and easily documentable) as possible, and of course also as logical as possible.

You know the implications for the rest of the code, so you can decide. I am fine with both solutions.

Whatever we decide, it should be mentioned in the migration guide (even if the current behaviour is not documented...), and the docs have to be updated. I can update the docs, but I don't have edit rights to the migration guide, so someone else will have to handle that.