Skip to content

Image fit features #5784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Sep 15, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/core/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -725,3 +725,15 @@ export const LABEL = 'label';
* @final
*/
export const FALLBACK = 'fallback';

/**
* @property {String} CONTAIN
* @final
*/
export const CONTAIN = 'contain';

/**
* @property {String} COVER
* @final
*/
export const COVER = 'cover';
133 changes: 127 additions & 6 deletions src/image/loading_displaying.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,73 @@ function _createGif(
finishCallback();
}

/**
* @private
* @param {Constant} xAlign either LEFT, RIGHT or CENTER
* @param {Constant} yAlign either TOP, BOTTOM or CENTER
* @param {Number} dx
* @param {Number} dy
* @param {Number} dw
* @param {Number} dh
* @param {Number} sw
* @param {Number} sh
* @returns {Object}
*/

function _imageContain(xAlign, yAlign, dx, dy, dw, dh, sw, sh) {
const r = Math.max(sw / dw, sh / dh);
const [adjusted_dw, adjusted_dh] = [sw / r, sh / r];
let x = dx;
let y = dy;

if (xAlign === constants.CENTER) {
x += (dw - adjusted_dw) / 2;
} else if (xAlign === constants.RIGHT) {
x += dw - adjusted_dw;
}

if (yAlign === constants.CENTER) {
y += (dh - adjusted_dh) / 2;
} else if (yAlign === constants.BOTTOM) {
y += dh - adjusted_dh;
}
return { x, y, w: adjusted_dw, h: adjusted_dh };
}
/**
* @private
* @param {Constant} xAlign either LEFT, RIGHT or CENTER
* @param {Constant} yAlign either TOP, BOTTOM or CENTER
* @param {Number} dw
* @param {Number} dh
* @param {Number} sx
* @param {Number} sy
* @param {Number} sw
* @param {Number} sh
* @returns {Object}
*/

function _imageCover(xAlign, yAlign, dw, dh, sx, sy, sw, sh) {
const r = Math.max(dw / sw, dh / sh, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just noticed, if you use an image that's larger than the destination, it doesn't scale down to minimally cover (because that would involve a scale lower than 1.) I think if we remove the , 1 here then it will handle that case though!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix this, I assumed that minimal cover was not required. But, looking at css cover it does actually minimally covers the larger image.

const [adjusted_sw, adjusted_sh] = [dw / r, dh / r];

let x = sx;
let y = sy;

if (xAlign === constants.CENTER) {
x += (sw - adjusted_sw) / 2;
} else if (xAlign === constants.RIGHT) {
x += sw - adjusted_sw;
}

if (yAlign === constants.CENTER) {
y += (sh - adjusted_sh) / 2;
} else if (yAlign === constants.BOTTOM) {
y += sh - adjusted_sh;
}

return { x, y, w: adjusted_sw, h: adjusted_sh };
}

/**
* Validates clipping params. Per drawImage spec sWidth and sHight cannot be
* negative or greater than image intrinsic width and height
Expand Down Expand Up @@ -402,6 +469,9 @@ function _sAssign(sVal, iVal) {
* rectangle
* @param {Number} [sHeight] the height of the subsection of the
* source image to draw into the destination rectangle
* @param {Constant} [fit] either CONTAIN or COVER
* @param {Constant} [xAlign] either LEFT, RIGHT or CENTER
* @param {Constant} [yAlign] either TOP, BOTTOM or CENTER
*/
p5.prototype.image = function(
img,
Expand All @@ -412,25 +482,30 @@ p5.prototype.image = function(
sx,
sy,
sWidth,
sHeight
sHeight,
fit,
xAlign,
yAlign
) {
// set defaults per spec: https://goo.gl/3ykfOq

p5._validateParameters('image', arguments);

let defW = img.width;
let defH = img.height;
yAlign = yAlign || constants.CENTER;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is CENTER if not specified by user

xAlign = xAlign || constants.CENTER;

if (img.elt && img.elt.videoWidth && !img.canvas) {
// video no canvas
defW = img.elt.videoWidth;
defH = img.elt.videoHeight;
}

const _dx = dx;
const _dy = dy;
const _dw = dWidth || defW;
const _dh = dHeight || defH;
let _dx = dx;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the fit mode needs to change these variables to allow fitting the entire image

let _dy = dy;
let _dw = dWidth || defW;
let _dh = dHeight || defH;
let _sx = sx || 0;
let _sy = sy || 0;
let _sw = sWidth || defW;
Expand Down Expand Up @@ -461,7 +536,53 @@ p5.prototype.image = function(
_sh *= pd;
_sw *= pd;

const vals = canvas.modeAdjust(_dx, _dy, _dw, _dh, this._renderer._imageMode);
let vals = { x: _dx, y: _dy, h: _dh, w: _dw };
if (fit) {
switch (this._renderer._imageMode) {
Copy link
Contributor

@davepagurek davepagurek Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is close to what we want! I think the image mode would change the destination box size (and shift the other way for CENTER, i.e.:

switch (this._renderer._imageMode) {
  case constants.CENTER:
    _dx -= _dw * 0.5;
    _dy -= _dh * 0.5;
    break;
  case constants.CORNERS:
    _dw -= _dx;
    _dh -= _dy;
    break;
}

...although I wonder if we can reduce the complexity by using modeAdjust, since that does these adjustments already? Something like this, where you pass vals.w, vals.h, vals.x and vals.y for dw, dh, dx, and dy:

const vals = canvas.modeAdjust(_dx, _dy, _dw, _dh, this._renderer._imageMode);

if (fit) {
  if (fit === constants.COVER) {
    const { x, y, w, h } = _imageCover(
        xAlign,
        yAlign,
        vals.w, // Replacing dw
        vals.h, // Replacing dh
        _sx,
        _sy,
        _sw,
        _sh
      );
      // etc

Copy link
Contributor Author

@manpreeeeeet manpreeeeeet Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using the existing modeAdjust function but the way it works is by shifting the dx or dy so as the image appears to be drawn from the center. The reason I couldn't use it to is because it in fitMode COVER moving away the destination box makes the image not cover the whole required box, which seems a bit counter intuitive.

Since from the definition:

imageMode(CENTER) interprets the second and third parameters of image() as the image's center point. If two additional parameters are specified, they are used to set the image's width and height.

Should the user expect the destination width be adjusted/[half in case of CENTER] of what they specified in fitMode COVER?

Copy link
Contributor Author

@manpreeeeeet manpreeeeeet Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the current behavior is the desired one. I think the complexity can be addressed by taking this logic outside into one function call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fitMode COVER moving away the destination box makes the image not cover the whole required box

I think sx and sy, sw, and sh only change what part of the source image gets put into the destination box, wherever the destination box is, so when you shift the destination box, the source image shifts with it. This would be consistent with how imageMode works with image when you don't provide any fit parameters: changing from imageMode(CORNER) to imageMode(CENTER) just shifts where the image is drawn, but the image itself is the same as before.

Let me know if I'm misinterpreting what your concern is, but I think your _imageCover and _imageContain functions will produce the expected output if we give them updated dx, dy, dw, and dh values, either from manually applying shifts as we're doing currently, or by giving them the outputs of modeAdjust.

case constants.CENTER:
_sx += _sw * 0.5;
_sy += _sh * 0.5;
break;
case constants.CORNERS:
_sw -= _sx;
_sh -= _sy;
break;
}

if (fit === constants.COVER) {
const { x, y, w, h } = _imageCover(
xAlign,
yAlign,
_dw,
_dh,
_sx,
_sy,
_sw,
_sh
);
_sx = x;
_sy = y;
_sw = w;
_sh = h;
} else if (fit === constants.CONTAIN) {
const { x, y, w, h } = _imageContain(
xAlign,
yAlign,
_dx,
_dy,
_dw,
_dh,
_sw,
_sh
);
vals.x = x;
vals.y = y;
vals.w = w;
vals.h = h;
}
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageMode shouldn't affect the image shown when fit mode is specified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want imageMode to affect positioning when fit is specified, and it should just refine the positioning. of the source image within the destination box specified. e.g. starting from this non-fit code:

rectMode(CENTER)
imageMode(CENTER)
rect(0, 0, dw, dh)
image(img, 0, 0, dw, dh, 0, 0, sw, sh)

adding some fit parameters, I would expect it to look like this:

rectMode(CENTER)
imageMode(CENTER)
rect(0, 0, dw, dh)
image(img, 0, 0, dw, dh, 0, 0, sw, sh, COVER, RIGHT, CENTER)

but without taking into account imageMode, it looks like this currently:

rectMode(CENTER)
imageMode(CENTER)
rect(0, 0, dw, dh)
image(img, 0, 0, dw, dh, 0, 0, sw, sh, COVER, RIGHT, CENTER)

Copy link
Contributor Author

@manpreeeeeet manpreeeeeet Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I think I get what you were trying to convey!

Currently, it doesn't take imageMode into account and produces the result as if imageMode is always CORNER. I was not able to reproduce this image going out of bound. I would appreciate if you could send me the src for this sketch. Also, my question regarding imageMode is if imageMode is CENTER and the user inputs
image(img, 0, 0, dw, dh, 0, 0, sw, sh, COVER, RIGHT, CENTER should we consider yAlign: center from the center of the image.

vals = canvas.modeAdjust(_dx, _dy, _dw, _dh, this._renderer._imageMode);
}

// tint the image if there is a tint
this._renderer.image(img, _sx, _sy, _sw, _sh, vals.x, vals.y, vals.w, vals.h);
Expand Down
53 changes: 53 additions & 0 deletions test/unit/image/loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -443,3 +443,56 @@ suite('displaying images', function() {
checkTint(tintColor);
});
});

suite('displaying images that use fit mode', function() {
var myp5;

setup(function(done) {
new p5(function(p) {
p.setup = function() {
myp5 = p;
done();
};
});
});

teardown(function() {
myp5.remove();
});

test('CONTAIN when source image is larger than destination', function() {
let src = myp5.createImage(400, 1000);
sinon.spy(myp5._renderer, 'image');
myp5.image(src, 0, 0, 300, 400, 0, 0, 400, 1000, myp5.CONTAIN);
assert(myp5._renderer.image.calledOnce);
assert.equal(myp5._renderer.image.getCall(0).args[7], 400 / (1000 / 400)); // dw
assert.equal(myp5._renderer.image.getCall(0).args[8], 1000 / (1000 / 400)); // dh
});

test('CONTAIN when source image is smaller than destination', function() {
let src = myp5.createImage(40, 90);
sinon.spy(myp5._renderer, 'image');
myp5.image(src, 0, 0, 300, 500, 0, 0, 400, 1000, myp5.CONTAIN);
assert(myp5._renderer.image.calledOnce);
assert.equal(myp5._renderer.image.getCall(0).args[7], 40 / (90 / 500)); // dw
assert.equal(myp5._renderer.image.getCall(0).args[8], 90 / (90 / 500)); // dh
});

test('COVER when source image is larger than destination', function() {
let src = myp5.createImage(400, 1000);
sinon.spy(myp5._renderer, 'image');
myp5.image(src, 0, 0, 300, 400, 0, 0, 400, 1000, myp5.COVER);
assert(myp5._renderer.image.calledOnce);
assert.equal(myp5._renderer.image.getCall(0).args[3], 300); // sw
assert.equal(myp5._renderer.image.getCall(0).args[4], 400); // sh
});

test('COVER when source image is smaller than destination', function() {
let src = myp5.createImage(20, 100);
sinon.spy(myp5._renderer, 'image');
myp5.image(src, 0, 0, 300, 400, 0, 0, 20, 100, myp5.COVER);
assert(myp5._renderer.image.calledOnce);
assert.equal(myp5._renderer.image.getCall(0).args[3], 20); // sw
assert.equal(myp5._renderer.image.getCall(0).args[4], 400 / 15); // sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on these tests!

});
});