EyalAr / lwip

Light Weight Image Processor for NodeJS
MIT License
2.37k stars 230 forks source link

confusing 'crop' paramters #231

Closed jomo closed 8 years ago

jomo commented 8 years ago

Currently the parameters for the crop() function are described as:

left, top, right, bottom {Integer}: Coordinates of the crop rectangle.

This suggest there need to be 4 coordinates (2 integers each) for all 4 corners. That is not true. The parameters are actually described as:

I recommend using these parameter names instead to avoid confusion.

EyalAr commented 8 years ago
This suggest there need to be 4 coordinates (2 integers each)

There are 4 parameters, not 8... "top-left" might describe a corner, but "left" does not.

Thanks for the comment @jomo, but I don't see the confusion. I'll definitely consider changing the terminology if more people find it confusing.

jomo commented 8 years ago

There are 4 parameters described as 'Coordinates', but 'Integers' at the same time. Coordinates are always a pair of two Integers, that's why I found it confusing.

Most confusing, however is that 'right' or 'bottom' is not a very descriptive parameter name, because they're the x and y position (counted from left and top, not right and bottom).

Take a look at this table:

left top right bottom
x0 y0 x1 y1

It's not obvious how those names map to their actual meaning.

right-bottom-x, right-bottom-y would make sense, but right, bottom does not. You could also use left0, top0, left1, top1.

The parameter names used by cimg are much more descriptive because it makes clear:

  1. xn and yn are a pair of coordinates
  2. they're x and y coordinates

Sorry if it wasn't clear what I meant in the first comment.