Image fit features #5784

manpreeeeeet · 2022-09-03T00:17:46Z

Addresses #5720
Changes:
Allows users to CONTAIN/COVER the specified destination box without distorting the aspect ratios.

Screenshots of the change:
CONTAIN
CONTAIN CENTER CENTER

CONTAIN CENTER TOP

COVER
COVER CENTER CENTER

COVER RIGHT CENTER

PR Checklist

npm run lint passes
[Inline documentation] is included / updated
[Unit tests] are included / updated

welcome · 2022-09-03T00:17:48Z

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

manpreeeeeet · 2022-09-03T00:29:09Z

This is a working prototype. It doesn't include unit-tests, since I couldn't think of what the tests should look like. Here's how the logic works.

CONTAIN
Since we, want to contain the entire image we find the img/destination ratio that is the biggest and decrease the destination box's width and height to the required ratio to allow the whole image to be shown with maximum allowed height or width.

COVER
This works a little differently from the CONTAIN since for images that are smaller than the destination box, we cannot get away with increasing the destination box's width/height to scale the image since it will not be what the user would expect when specifying the dw, dh we were able to change these values in CONTAIN cause they were smaller than user specified and didn't interrupt with the desired behavior. So, the image needs to be resized by the minimum factor of the ratios as to cover the whole container.

I am fairly new to open-source so any suggestions on how to improve this are welcome.

manpreeeeeet · 2022-09-03T00:35:33Z

src/image/loading_displaying.js

 ) {
  // set defaults per spec: https://goo.gl/3ykfOq

  p5._validateParameters('image', arguments);

  let defW = img.width;
  let defH = img.height;
+  yAlign = yAlign || constants.CENTER;


default is CENTER if not specified by user

manpreeeeeet · 2022-09-03T00:36:30Z

src/image/loading_displaying.js

-  const _dy = dy;
-  const _dw = dWidth || defW;
-  const _dh = dHeight || defH;
+  let _dx = dx;


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

manpreeeeeet · 2022-09-03T00:37:24Z

src/image/loading_displaying.js

+      vals.w = w;
+      vals.h = h;
+    }
+  } else {


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

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)

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.

davepagurek · 2022-09-03T18:23:16Z

src/image/loading_displaying.js

+
+function _imageCover(img, xAlign, yAlign, dw, dh, sx, sy, sw, sh) {
+  const r = Math.max(dw / sw, dh / sh, 1);
+  img.resize(img.width * r, img.height * r);


I think we aren't able to use img.resize in this method because not all input types have this method (e.g. p5.MediaElement, the return type of createVideo). Also, resize modifies the image object, and I think we want to keep these methods from modifying the input.

davepagurek · 2022-09-03T18:33:39Z

src/image/loading_displaying.js

+ */
+
+function _imageContain(xAlign, yAlign, dx, dy, dw, dh, sw, sh) {
+  const r = Math.max(sw / dw, sh / dh, 1);


We might need to get the value of r before doing the Math.max(..., 1) in order to handle the case where the source image is smaller than the destination box and needs to be scaled up past its initial size. Right now, if I add a tiny source image (20x20 in this example), it never draws larger than its initial size, but we'll want it to uniformly scale up to fit the destination box:

img.resize(20, 20) rect(0, 0, dw, dh) image(img, 0, 0, dw, dh, 0, 0, sw, sh, CONTAIN, RIGHT, CENTER)

Expected Current

It calculates, the minimum needed ratio that we need to scale the destination box by, so that we don't distort the image aspect ratio. Let me, know if it still confuses you.

davepagurek · 2022-09-03T18:35:39Z

Thanks for taking this on, and nice work on this! I tested different combinations of fits and fills and the alignment logic works well! I left a few comments about handling more edge cases. Let me know if I can clarify anything!

davepagurek · 2022-09-03T19:11:23Z

Also, here's an idea of how we can unit test this: All image() calls end up calling this._renderer.image() at the end. Maybe we can make assertions on the parameters that get passed to that function? We could do this with spies, something like:

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

davepagurek · 2022-09-11T20:49:31Z

src/image/loading_displaying.js

@@ -539,6 +539,17 @@ p5.prototype.image = function(

  let vals = { x: _dx, y: _dy, h: _dh, w: _dw };
  if (fit) {
+    switch (this._renderer._imageMode) {


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

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?

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

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.

davepagurek · 2022-09-11T20:51:27Z

test/unit/image/loading.js

+    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


Great work on these tests!

davepagurek · 2022-09-11T21:02:00Z

src/image/loading_displaying.js

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


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!

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

fix: updated tests improved: readability

davepagurek · 2022-09-14T11:59:22Z

src/image/loading_displaying.js

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


Nice, this code looks great!

davepagurek · 2022-09-14T12:03:16Z

I'll run the code again this evening to double check that everything works as expected, but unless anything new comes up, this seems good to go! Great work!

The one final thing we might want to do is add an example to the comments of image() showing usage of fitting so that readers of the p5 docs can see how it might be used. Maybe we can add an example sketch that shows one COVER and one CONTAIN so they can see the difference?

davepagurek · 2022-09-15T01:30:41Z

Just played around with it some more, this looks good to me! I think once we have an example in the docs, this is good to go 😁

davepagurek · 2022-09-15T12:04:14Z

src/image/loading_displaying.js

@@ -424,6 +424,14 @@ function _sAssign(sVal, iVal) {
 * to explain further:
 * <img src="assets/drawImage.png"></img>
 *
+ * This function can also be used to draw images without distorting the orginal aspect ratio,
+ * by adding 9th parameter fit which can either be COVER or CONTAIN.


Super minor grammar suggestion: I think a few commas on this line and the next will make this a tad easier to read:

- * by adding 9th parameter fit which can either be COVER or CONTAIN. + * by adding 9th parameter, fit, which can either be COVER or CONTAIN. - * CONTAIN as the name suggests contains the whole image within the specified destination box + * CONTAIN, as the name suggests, contains the whole image within the specified destination box

davepagurek · 2022-09-15T12:07:21Z

src/image/loading_displaying.js

+ * function setup() {
+ *   // COVER the whole destination box without distorting the image's aspect ratio
+ *   // COVER the specified destination box which is of dimension 100 x 100
+ *   // Display the CENTER part of the image for both LEFT and TOP corner


Is this comment to remind readers that CENTER is the default for x and y align? If so, maybe this wording will be a little clearer:

// Without specifying xAlign or yAlign, the image will be // centered in the destination box in both axes

Thanks for the suggestion, this makes it a lot more easier to read.

davepagurek · 2022-09-15T12:08:09Z

Thanks for adding those examples!

davepagurek

I think this is all ready to go now! Thanks for the time you put into this feature 🙂

davepagurek · 2022-09-15T20:33:24Z

@all-contributors please add @Manpreet-Singh001 for doc, code, test

allcontributors · 2022-09-15T20:33:33Z

@davepagurek

I've put up a pull request to add @Manpreet-Singh001! 🎉

manpreeeeeet added 7 commits August 20, 2022 13:48

intial shrink functionality setup

def7f83

initial setup and adding parameters

ba8c237

Merge branch 'main' into image-fit-features

92fc843

working features, need comments

9aacc83

Merge branch 'main' into image-fit-features

12c369c

Merge branch 'main' into image-fit-features

52745f1

removes the left-over console.log

25876ca

removed the typo for valid params for yAlign

0d57a8e

manpreeeeeet commented Sep 3, 2022

View reviewed changes

Qianqianye requested review from endurance21 and davepagurek and removed request for endurance21 September 3, 2022 03:36

davepagurek reviewed Sep 3, 2022

View reviewed changes

manpreeeeeet added 5 commits September 3, 2022 22:41

contain now scales the smaller image up

d58e658

removes the need for img.resize() plus further cleaning

a0ed0ba

takes imageMode into account

9b4a40c

removed the leftover console.log

caf3692

basic tests

03c8d38

davepagurek reviewed Sep 11, 2022

View reviewed changes

fix: minimal contains larger images in cover

9ad9365

fix: updated tests improved: readability

davepagurek reviewed Sep 14, 2022

View reviewed changes

docs: added docs for the fit mode

89c48fb

davepagurek reviewed Sep 15, 2022

View reviewed changes

manpreeeeeet added 2 commits September 15, 2022 06:24

grammar: typos and grammar fix

40edc0f

grammar: typo

4336765

davepagurek approved these changes Sep 15, 2022

View reviewed changes

davepagurek merged commit 5a44d40 into processing:main Sep 15, 2022

allcontributors bot mentioned this pull request Sep 15, 2022

add Manpreet-Singh001 as a contributor #5803

Merged

processing deleted a comment from allcontributors bot Sep 15, 2022

renovate bot mentioned this pull request Oct 18, 2022

Update dependency p5 to v1.5.0 bunhojun/draw-what-svelte#246

Merged

1 task

renovate bot mentioned this pull request Oct 26, 2022

fix(deps): update dependency p5 to v1.5.0 WolfSoko/wol-sok-mono#13

Merged

1 task

renovate bot mentioned this pull request Nov 7, 2022

chore(deps): update dependency p5 to v1.5.0 zenoplex/react-use-p5#404

Merged

1 task

This was referenced Nov 20, 2022

Update dependency p5 to v1 - abandoned ff6347/gestalten-in-code#71

Open

Update dependency p5 to v1.11.5 nszknao/generative-art#17

Open

Update dependency p5 to v1.5.0 ibveecnk/oldwebsite#219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image fit features #5784

Image fit features #5784

manpreeeeeet commented Sep 3, 2022 •

edited

Loading

welcome bot commented Sep 3, 2022

manpreeeeeet commented Sep 3, 2022 •

edited

Loading

manpreeeeeet Sep 3, 2022

manpreeeeeet Sep 3, 2022

manpreeeeeet Sep 3, 2022

davepagurek Sep 3, 2022

manpreeeeeet Sep 3, 2022 •

edited

Loading

davepagurek Sep 3, 2022

davepagurek Sep 3, 2022 •

edited

Loading

manpreeeeeet Sep 3, 2022

davepagurek commented Sep 3, 2022

davepagurek commented Sep 3, 2022 •

edited

Loading

davepagurek Sep 11, 2022 •

edited

Loading

manpreeeeeet Sep 11, 2022 •

edited

Loading

manpreeeeeet Sep 11, 2022 •

edited

Loading

davepagurek Sep 11, 2022

davepagurek Sep 11, 2022

davepagurek Sep 11, 2022

manpreeeeeet Sep 11, 2022

davepagurek Sep 14, 2022

davepagurek commented Sep 14, 2022

davepagurek commented Sep 15, 2022

davepagurek Sep 15, 2022

davepagurek Sep 15, 2022

manpreeeeeet Sep 15, 2022

davepagurek commented Sep 15, 2022

davepagurek left a comment

davepagurek commented Sep 15, 2022

allcontributors bot commented Sep 15, 2022

Image fit features #5784

Image fit features #5784

Conversation

manpreeeeeet commented Sep 3, 2022 • edited Loading

PR Checklist

welcome bot commented Sep 3, 2022

manpreeeeeet commented Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manpreeeeeet Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Sep 3, 2022

davepagurek commented Sep 3, 2022 • edited Loading

davepagurek Sep 11, 2022 • edited Loading

Choose a reason for hiding this comment

manpreeeeeet Sep 11, 2022 • edited Loading

Choose a reason for hiding this comment

manpreeeeeet Sep 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Sep 14, 2022

davepagurek commented Sep 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Sep 15, 2022

davepagurek left a comment

Choose a reason for hiding this comment

davepagurek commented Sep 15, 2022

allcontributors bot commented Sep 15, 2022

manpreeeeeet commented Sep 3, 2022 •

edited

Loading

manpreeeeeet commented Sep 3, 2022 •

edited

Loading

manpreeeeeet Sep 3, 2022 •

edited

Loading

davepagurek Sep 3, 2022 •

edited

Loading

davepagurek commented Sep 3, 2022 •

edited

Loading

davepagurek Sep 11, 2022 •

edited

Loading

manpreeeeeet Sep 11, 2022 •

edited

Loading

manpreeeeeet Sep 11, 2022 •

edited

Loading