Wrong calculation of bounding rect

[lukejagodzinski]

Hi, I was using function getBoundingRect on the object to detect its width and height but it wasn't real value. Having image of size 200x200 pixels, from getBoudingRect() I get width 200.5 and the same with height. After investigation I got to conclusion that it's because of setCoords function takes this.strokeWidth into calculations even though this.stroke property is undefined, so there is no stroke rendered. I had to set strokeWidth to 0 and now calculations are made correctly. Maybe it's not big deal but I would propose two solutions. The default value for strokeWidth property should be 0 or setCoords function should count strokeWidth only when stroke is visibile:

setCoords: function() {
  var strokeWidth = typeof this.stroke === 'string' ? this.strokeWidth : 0,
  /* ... */
}

The ideal solution would be to do both.

[asturur]

I think you are using latest version,
because i remember this calculation has been addressed recently.

probably if we start with this.stroke without color, strokewidth should start with 0 as you say, for consistency. As for now it is still set to 1.

But implying "no color" => "no width" is not right to me, why changing a color i should expect a change in dimensions? as from a user perspective is strange, don't you think?

Chek PR #1882 there are some changes proposed, some are already merged.

[lukejagodzinski]

Yes I'm using the newest version.

Hmm yep, maybe you are right. So probably the best option would be to set it to 0 by default. I may consider situation where developer want to have transparent stroke for a moment and still calculate dimensions including stroke. And later he/she can set stroke opaque.

So is it going to be 0 value by default in coming PR?

I'm using Fabric.js a lot lately and I can contribute. I can make PRs if I find some errors.

[asturur]

@kangax evaluates directions / decisions.
It would not be bad to include this little change even in that coming PR #1882.
But even if it looks little, for backward compatibility is a huge change. everyone how never set the strokewidth ( and decide to update fabric ) will have border disappear.

any PR is always welcome. That it doesn't mean it gets merged always, a brief discussion before is welcome even more.

[lukejagodzinski]

Yes right backward compatibility is always a problem. It can mess a lot. I can think of solution for that but it's not perfect one. If someone was using strokes so he/she for sure set stroke property. We could use JS setters to detect setting stroke property and while setting it check if strokeWidth is equal 0, if so set it to 1. I know it's bad solution but only one that wouldn't break people's codes.

Ok so I will discuss and contribute :)

[asturur]

@kangax we do not need any confirmation for this.
A 200 width rect will have a bounding box of 201px ( 0.5 of border per side, other 0.5 is inside the rect).
Just decide if for 1.5.0 release you want to put default colored border OR default border 0 pixel width.

So we clear confusion.

[kangax]

Good question. I'm really not sure which one we should return. I'm leaning towards 200 in case like this. What do you think @jagi @asturur @Kienz

[asturur]

i m for counting border even if no fill.

and for changing default strokewidth to 0 or default stroke to black.

[kangax]

Black or transparent? :)

[asturur]

or strokewdith 1 and stroke black
or strokewidth 0 and stroke transparent

one of this couples

[kangax]

Let's go with "strokewidth 0, transparent"?

[asturur]

There are lot of places in the code where we evaluate:
strokeWidth = this.stroke ? this.strokeWidth : 0

Can i start to hunt them?

[kangax]

Yes!