polygon bounding box issue

[ericmandel]

We at the Harvard-Smithsonian Center for Astrophysics are developing the next generation of astronomical image display: a Web-based version of the de facto standard DS9 (desktop) program. We are using fabric.js to manage our line graphics: regions of interest, astronomical catalogs, iso-intensity contour maps, etc. See: http://js9.si.edu for the current (beta) release. A few issues have arisen regarding fabric, including:

Our contour maps show areas of constant intensity and are displayed using a large number of polygons. The position of each vertex in each polygon must be accurate, even at high zoom (scale) levels. But by default, fabric changes the position of the vertices to fit into the bounding box (_calcDimensions()). Therefore we pass "true" as the third argument to new Fabric.Polygon, telling fabric to skipOffset. However, doing this results in a polygon that does not fit inside the bounding box.

This can easily be seen in the following fiddle: http://jsfiddle.net/r96Tp/ The vertices of the red polygon are modified by _calcDimensions() and fit into the bounding box. The vertices of the green polygon are not modified (skipOffset set to true) and do not fit. This problem happens in fabric 1.4.0 and 1.4.8.

Is this a bug or a feature? If the latter, is there a simple work-around to recalculate the bounding box parameters without changing the vertex positions?

Regards,

Eric Mandel

P.S. I should mention that we recently switched from kinetic.js to fabric.js because of your SVG support, which is essential for scalable printing. Very nice work!

[asturur]

i'm messing up with polygons and bounding boxes. @asturur so i will find it easy.

[kangax]

@asturur is this still an issue?

[asturur]

@ericmandel

could you try now the latest version? vertices get translated but then they are put back in place. you should be able to have them in correct position now.

or if you cannot try please post some code with wich i can make test for you.

skipoffset parameter has been removed.

[ericmandel]

@asturur

Does the original fiddle http://jsfiddle.net/r96Tp/ now work with your new code? It should show the two polygons at the initial location and the bounding box should be correctly placed for each. If that works, I can try to make a test of our JS9 app next week.

[asturur]

I just didn't know what you expected from the fiddle. anyway: http://jsfiddle.net/r96Tp/2/ this is modified with last version. Now the triangle have both the points modified and are placed on the top left you asked with the script 50 , 50. Whatever points you will input they will modified to get near the origin and then they will be moved according to occupy theyr original position with a calculated top and left. Otherwise you specify top and left ( as you did ) and the polygon will be where you asked and not in the original location of theyr points. To have original location do not specify top and left. Bounding box should be corrected now for every polygon or polyline. So just the vector between couples of points has real meaning.

I don't know if this is what you wanted, but this is how it will behave in the future.

Please tell us if is fine, so we can close this issue. Regards.

[ericmandel]

I must be misunderstanding something, because this seems to be the exact opposite of what we need. In your fiddle, the 3 points start out as {-4.75,1.667}, {-1.1,-1.667}, {5.85,0} relative to the 50,50 top/left origin, and end up as {-5.3,1.667}, {-1.65,-1.667}, {5.3,0} after scaling.

But we need the polygon point positions to be exactly the same after scaling, relative to the origin. These points have been calculated very accurately with regard to features in the data. When the points are moved after scaling, they are no longer centered on these features, i.e. the contour lines are just wrong relative to the data.

From your explanation, I can't quite understand if there is a way to keep the relative positions of the points from changing, as was the case when we set skipoffset to true. Can you please restate what we should do?

Regards,

Eric

[asturur]

when you create a polygon points are offseted. so 5,1 10,2 8,12 becomes 0,0 5,1 3,11 and top left get assigned to 1,5.

so the points will be drawed in original coords.

if you specify 50 50 points will be drawn to 55,51 60,52 58,62.

now scaling: if you scale by 2 the coordinates are scaled respect origin ( 0,0 ) , so x and y get scaled by 2 but top and left are mantained.

so polygon will be 10,2 20,4 16,24 but top left stays the same. so there is not centered scaling

this is how it should behave, and i hope it does. if not, there is some bug and it will be fixed.

[ericmandel]

Please look carefully at the shape1 processing in your fiddle: http://jsfiddle.net/r96Tp/2/ This is what I see. Perhaps I am missing something:

1. The shape1 polygon is created at 50,50 with an array of points [ {-4.75,1.667}, {-1.1,-1.667}, {5.85,0}] passed as the first argument to the Polygon call.
2. The shape1 polygon is scaled by a factor of 32.
3. Then the values of the same shape1.points array are printed out and some of the x,y values are different from their original counterparts by a fraction of a pixel: {-5.3,1.667}, {-1.65,-1.667}, {5.3,0} after scaling. The change is on the order of .5 pixels.

It's this fraction of a pixel change that is the problem. I do not expect the x,y values in the points array to change relative to the center when you scale, just as the radius of a circle does not change when you scale and the length and width of a box do not change, etc.

In the original fabric 1.4.0 (and 1.4.8) code, these x,y values were explicitly changed in _calcDimensions() to fit the polygon into the bounding box unless the skipoffset parameter was set to true. We set skipOffset to avoid changing the x,y values -- but then the polygon did not fit into the bounding box. This was my original report.

Now it seems like the points still are being changed when the polygon is scaled, but without the possibility of setting the skipOffset flag. But, again, perhaps I am missing something.

Finally, please note that you should not check this behavior using integer values for the points. I believe when I originally looked into this, I found that the fractional difference does not happen if the points are integers. The bizarre point values chosen for the fiddle are there to show the behavior clearly.

[asturur]

thanks Eric, i re read all conversation and there was no indication of this fractions / rounding problem.

There are a lot of transform in the process, i m gonna check if there is some rounded operation and why or if is just javascript rounding bad.

[ericmandel]

I don't think this is a rounding issue, since it is very explicit in the 1.4.[0,8] _calcDimensions() code that the vertices are being changed to fit into the bounding box:

  if (skipOffset) return;
  var halfWidth = this.width / 2 + this.minX,
      halfHeight = this.height / 2 + this.minY;
  // change points to offset polygon into a bounding box
  this.points.forEach(function(p) {
    p.x -= halfWidth;
    p.y -= halfHeight;
  }, this);

Hence the use of skipOffset to avoid that change. But let me know if I can help.

[asturur]

vertices need to be changed for all the pieces of library to work ( bounding box , gradients, patterns , svgimport and export ).

i miss the background of what your application does and how. so i m far , but willing, to help you.

the issue of bounding box is closed for me, but if you drop me a mail at andreabogazzi79@gmail.com i can see if i can understand. i need to understand what you do with the points array and why you want them original, if it is for reading them back, for your application calculation, maybe we can store in other array and keep there for reference like we do with original path in path class. Il 05/set/2014 20:12 "Eric Mandel" notifications@github.com ha scritto:

I don't think this is a rounding issue, since it is very explicit in the 1.4.[0,8] _calcDimensions() code that the vertices are being changed to fit into the bounding box:

if (skipOffset) return; var halfWidth = this.width / 2 + this.minX, halfHeight = this.height / 2 + this.minY; // change points to offset polygon into a bounding box this.points.forEach(function(p) { p.x -= halfWidth; p.y -= halfHeight; }, this);

Hence the use of skipOffset to avoid that change. But let me know if I can help.

— Reply to this email directly or view it on GitHub https://github.com/kangax/fabric.js/issues/1553#issuecomment-54661190.

[asturur]

This issue is solved.

[mlshvdv]

But where is the solution?

[asturur]

It has been pulled in. The issue was named polygon Bounding Box , but the problem was elsewhere.

It was about how polygon vertices are changed and why they are changed. Pull request for reference is : #1635 and #1639

[mlshvdv]

Thank you.

30.09.2014, 15:24, "Andrea Bogazzi" notifications@github.com:

It has been pulled in. The issue was named polygon Bounding Box , but the problem was elsewhere.

It was about how polygon vertices are changed and why they are changed. Pull request for reference is : #1635 and #1639

— Reply to this email directly or view it on GitHub.