Skip to content
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

Image Editor Batch API #23369

Closed
wants to merge 4 commits into from

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Jun 22, 2020

Description

This proposes a new API for image editing that should be more flexible and extensible than the existing API. The new API takes an array of modifiers that will be applied in the order they appear.

An example request body looks like this.

{
  "modifiers": [
    {
      "type": "crop",
      "left": 12.875,
      "top": 0.125,
      "width": 50,
      "height": 50
    },
    {
      "type": "rotate",
      "angle": 90
    }
  ]
}

Crop values are as a percentage of the original image and rotation angle is in degrees clockwise.

The crop values need to be percentages because the image loaded on the frontend may not be a scaled version of the original image, so naturalWidth and naturalHeight cannot be trusted as the actual width and height of the original image.

The rotation value is in degrees to avoid rounding errors for the types of rotations that are most common like 90 degrees. Clockwise versus anticlockwise may be up for debate. When using degrees it seems more natural to me to rotate clockwise, but mathematically, when you've converted to radians, a positive value indicates an anticlockwise rotation.

Different front-end implementations may apply edits in a different order, so passing an array will allow them to use their edits as-is without converting to a fixed order.

One example in the wild that actually uses both orders in different contexts is BeFunky. To see the difference, try one of their demo images with a horizon in it so you have a line that you can pay attention to. Then use the 'straighten' option followed by a 'horizontal flip' to see that the angle of the horizon hasn't changed even though you flipped the image. This means that the flip happens before the rotation. If you use the 'rotate' option instead of 'straighten, the rotation happens before the flip.

Original description

Applies just the REST API changes from #22959 and continues where the old PR left off for REST API changes. This PR reverts the API changes from 5886225 (#23284), so the diff may read better on ajlende#484 which includes only the API changes compared to the commit prior to 5886225.

  • Moves richimage route to image-editor
  • Moves /apply to / since there is only one image editor endpoint now
  • API takes an array of modifiers that can be applied in order instead of an object
  • Updates x and y to be left and top which is more descriptive so you know which edge the offset is based off of

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ajlende ajlende force-pushed the fix/image-editor-batch-api branch 2 times, most recently from ffc7e2e to 8f26876 Compare June 22, 2020 18:23
@ajlende ajlende mentioned this pull request Jun 22, 2020
6 tasks
@ajlende ajlende force-pushed the fix/image-editor-batch-api branch 2 times, most recently from c444b67 to d0ee6e7 Compare June 22, 2020 19:15
@ajlende ajlende marked this pull request as ready for review June 22, 2020 19:17
@ajlende ajlende requested a review from ellatrix June 22, 2020 19:17
@ellatrix
Copy link
Member

API takes an array of modifiers that can be applied in order instead of an object

Why is that needed?

@ajlende
Copy link
Contributor Author

ajlende commented Jun 22, 2020

API takes an array of modifiers that can be applied in order instead of an object

Why is that needed?

It's a more flexible API. Edits can happen in whatever order is required. The order of edits with react-easy-crop are rotate, flip, [scale,] crop, but if that was to be switched out the order may be different. Crop will almost certainly happen last, but I've seen rotating and flipping happen in different orders on other online editors.

@azaozz
Copy link
Contributor

azaozz commented Jun 22, 2020

Why is that needed?

As far as I see this is (mostly) a replacement for the current WP_Image_Editor class that organizes the code slightly differently. It doesn't seem needed for the image editor, and I'm not sure why the current WP_Image_Editor should be replaced?

@ajlende
Copy link
Contributor Author

ajlende commented Jun 22, 2020

I'm not sure why the current WP_Image_Editor should be replaced?

I don't see this as replacing WP_Image_Editor at all. This is managing file naming and metadata for the REST API that I don't think is relevant to WP_Image_Editor.

@ajlende ajlende changed the title Fix/image editor batch api Image Editor Batch API Jun 22, 2020
@TimothyBJacobs
Copy link
Member

I'm nervous about introducing such a large abstraction if we're targeting this for 5.5.

@azaozz
Copy link
Contributor

azaozz commented Jun 22, 2020

Thinking more about it, seems good to hash how this works in PHP (although that may be repeating stuff posted elsewhere).

The image editor "back-end" needs to receive a set of instructions on how the image file has to be edited. This is not necessarily all of the "editing actions" the user performed. The actions can be combined to reduce the number of "edit operations" that have to be performed on the image file. For example: rotate:90 + rotate:90 + rotate:270 would be combined into a rotate:90 before applying them to the image file. The combining can happen either in the editor UI (js) (just before saving, as the separate "editing actions" done by the user are needed for undo/redo), or on the server.

However it is not possible to combine rotate/flip when there is a crop in between. For example: rotate:90 + crop:10,15,30,40 + rotate:90 cannot be combined and need to be performed in the same order (rotating a 300x400 image by 90 degrees will result in a 400x300 image, so the crop has to be done after the rotation, etc.).

As the order of these instructions is important, don't think it is a good idea to separate them. As far as I see a good format for submitting the instructions to the PHP back-end would either be an array, or even better a CSV? Then the instructions would look like: rotate:90;crop:10,15,30,40;rotate:90 and in PHP it can do explode( ';', $value ) and apply all the edit operations one after the other to the image file.

Once the image file is edited on the server, a new attachment is created and the data for it will be returned through the API (with all the standard info like any other image attachment), and the image will be added to the Image Library (like after uploading a new image) and replaced in the image block in the editor.

@TimothyBJacobs
Copy link
Member

As far as I see a good format for submitting the instructions to the PHP back-end would either be an array, or even better a CSV?

The endpoint currently passes an array of instructions like this.

[
  {
    "modifier": "crop",
    "left": 0,
    "top": 10,
    "width": 100,
    "height": 50
  },
  {
    "modifier": "rotate",
    "angle": 90
  }
]

or even better a CSV?

I'd prefer this stay as an array, we lose the ability to validate with a schema if we add a custom serialization mechanism. What would be better about using this format rotate:90;crop:10,15,30,40;rotate:90? It seems far less descriptive to me.

@azaozz
Copy link
Contributor

azaozz commented Jun 22, 2020

The endpoint currently passes an array of instructions like this

Yeah, that works too but has somewhat... redundant data structure?

  {
    "modifier": "rotate",
    "angle": 90
  }

Why would it need a modifier and angle? If we are doing rotation, the only possible value is a number of degrees? We are dealing with a very limited set of possible "edit actions" here, only three (flip, rotate, crop). And the values for these actions are also very limited: either a single number or a set of four numbers (floats).

Anyway, the current format works too, just seems too "wordy" perhaps? :)

@azaozz
Copy link
Contributor

azaozz commented Jun 22, 2020

This is managing file naming and metadata for the REST API...

File naming is managed by WP_Image_Editor::generate_filename() and attachment meta is added and managed by wp_generate_attachment_metadata() which calls wp_create_image_subsizes().

As an edited image is saved as a new attachment, these would be the functions to use (this would also maintain full backwards compatibility)? Then the existing image in the image block in the editor will be replaced with the new attachment. The new attachment would (should) also be added to the Media Library modal. At this point it is a standard attachment, same as newly uploaded image.

Then if the user decides to edit it again, they will be editing the new attachment, etc.

@ajlende
Copy link
Contributor Author

ajlende commented Jun 23, 2020

However it is not possible to combine rotate/flip when there is a crop in between.

It is possible with a little math 🙂 I didn't get around to finishing that part in #22959. Your specific example

rotate:90 + crop:10,15,30,40 + rotate:90

could be simplified to rotate:180 + crop:(width-15-40),10,40,30.

comparison

My point about order of operations was more to match up with whatever was more convenient for the front-end if, for example, someone else wanted to use this API it with a different cropping library that assumed a different order of operations.

One example in the wild that actually uses both orders in different contexts is BeFunky. Try one of their demo images with a horizon in it so you have a line that you can pay attention to. Then use the 'straighten' option followed by a 'horizontal flip' to see that the angle of the horizon hasn't changed even though you flipped the image. This means that the flip happens before the rotation. If you use the 'rotate' option instead of 'straighten, the rotation happens before the flip.

Those changes could be simplified down too, but it would be more convenient to give the option to change the order and not have to manually convert to the way that the API handles it.

@ajlende
Copy link
Contributor Author

ajlende commented Jun 23, 2020

Yeah, that works too but has somewhat... redundant data structure?

  {
    "modifier": "rotate",
    "angle": 90
  }

You're right, it does seem somewhat redundant in this case, but I think it makes the request and the code a little more readable.

And, when building a new REST API endpoint, I think backwards compatibility (even though we're not considering other modifications at this time) should be a consideration because changing APIs are not fun for the developers who rely on them.

Filters, which we've discussed in earlier image editor PRs could be an example of a modification which might get added with keys which are much less obvious and could benefit from the readability of a modifier key.

{
  "dark": "#111",
  "light": "#EEE",
  "angle": 22
}

Maybe this modification is a gradient overlay that could replace the CSS version from the cover block. It includes the angle key just like a rotation. Not a huge problem—you just have to make sure that you check for the gradient filter before the rotation filter or ensure that angle is the only key for the rotation filter. A small opportunity for bugs, but it'll be fine.

Then maybe you add another filter that rotates the colors like you can do in Gimp or Photoshop. That modification also takes an angle key, but this time the angle key is the only argument required.

{
  "angle": 22
}

This is where pattern matching would break down. You could change the key to colorAngle or something. Or you could add another key like the modifier key to indicate that this is a color rotation instead of an image rotation.

I think having the modifier key would solve the problems above. It might not be strictly necessary now, but as a REST API it would maintain consistency with other image modifications while maintaining backwards compatibility as features are added.

@ajlende
Copy link
Contributor Author

ajlende commented Jun 23, 2020

File naming is managed by WP_Image_Editor::generate_filename() and attachment meta is added and managed by wp_generate_attachment_metadata() which calls wp_create_image_subsizes().

I didn't realize that. Your earlier comments about using WP_Image_Editor make so much more sense now. 🙂

So you think it would be better to update those methods along with WP_Image_Editor::crop() and the other modifiers to add the special metadata? I can start looking into that.

@ajlende
Copy link
Contributor Author

ajlende commented Jun 23, 2020

I'm nervous about introducing such a large abstraction if we're targeting this for 5.5.

I wouldn't say that it has to make it into 5.5 since the other API is still marked as experimental. These are my thoughts on what a stable API could look like.

@ajlende ajlende force-pushed the fix/image-editor-batch-api branch from c4a6e52 to d667c51 Compare June 30, 2020 23:55
@ajlende ajlende self-assigned this Jul 1, 2020
@ajlende ajlende added [Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media REST API Interaction Related to REST API labels Jul 1, 2020
@ajlende ajlende marked this pull request as ready for review July 1, 2020 00:10
@TimothyBJacobs
Copy link
Member

@ajlende what does the generated args look like at this point?

@ajlende
Copy link
Contributor Author

ajlende commented Jul 1, 2020

The top-level schema only checks for an array of objects containing any of the properties from any of the modifiers. And the type (formerly modifier) is now an enum instead of a const.

array(
  'modifiers' => array(
    'type'     => 'array',
    'required' => true,
    'items'    => array(
      'type'       => 'object',
      'properties' => array(
        'type' => array(
          'type'     => 'string',
          'enum'     => array( 'crop', 'rotate' ),
          'required' => true,
        ),
        'left'   => array(
          'type'     => 'number',
          'minimum'  => 0,
          'maximum'  => 100,
        ),
        'top'    => array(
          'type'     => 'number',
          'minimum'  => 0,
          'maximum'  => 100,
        ),
        'width'  => array(
          'type'     => 'number',
          'minimum'  => 1,
          'maximum'  => 100,
        ),
        'height' => array(
          'type'     => 'number',
          'minimum'  => 1,
          'maximum'  => 100,
        ),
        'angle' => array(
          'type'     => 'integer',
        ),
      ),
    ),
  ),
)

That's why I do a check later after I know if the type is crop or rotate for the individual required parameters using the schemas from get_crop_schema() and get_rotate_schema().

@TimothyBJacobs
Copy link
Member

Thanks! Do you want to take a crack at supporting oneOf and friends? We discussed it a bit in #core-restapi. The main thing is trying to figure out a way to print useful error messages.

@ajlende
Copy link
Contributor Author

ajlende commented Jul 1, 2020

Do you want to take a crack at supporting oneOf and friends?

I thought about it when making this change, but it seemed kind of intimidating which is why I took this approach. It would be helpful, though, so I can give it a shot if we use what I have as in interim solution.

@TimothyBJacobs
Copy link
Member

if we use what I have as in interim solution.

Are we trying to get this version of the endpoint for 5.5?

@ajlende
Copy link
Contributor Author

ajlende commented Jul 1, 2020

Are we trying to get this version of the endpoint for 5.5?

Originally I was thinking no, but with #23536 moving the endpoint out of experimental, I'd prefer to not have to break backwards compatibility with an API that's going to be in core. Not sure how far along moving the PHP over is going at this point, but we can discuss more at the #core-editor meeting if you're available for that.

@ellatrix
Copy link
Member

ellatrix commented Jul 1, 2020

I'm not opposed to allowing a different editing order, but I don't see value in allowing multiple edits of the same kind? That would create extra work on the server, which is maybe not obvious.
What if we add an order key that allows you to change the order of edits like: [ 'crop', 'flip', 'rotate' ]?

@ajlende
Copy link
Contributor Author

ajlende commented Jul 1, 2020

The one case that might make sense for multiple edits of the same kind is rotation and straighten as I mentioned in the description about BeFunky. They could be flattened down into one operation, but it would be more natural to keep them as separate. As @azaozz pointed out, the math to flatten down these operations can be quite complicated, especially around multiple crop operations. Let the API be more generic, and if needed, those optimizations can be added on the API side without breaking backwards compatibility. Yes, the request body may grow large if you wanted to just send every operation in order without flattening first, but it'll make the API easier to consume.

@TimothyBJacobs
Copy link
Member

I'd prefer to not have to break backwards compatibility with an API that's going to be in core.

My plan is to transform the old request format to the new format when we introduce these changes. But if we can stabilize the changes in time, that'd of course be preferable.

@ajlende ajlende force-pushed the fix/image-editor-batch-api branch from 11f389a to ab22cb1 Compare July 1, 2020 15:54
@ajlende
Copy link
Contributor Author

ajlende commented Jul 2, 2020

Thoughts on adjusting the API a bit from here? Having the type key mixed in with the rest of the arguments means that you wouldn't be able to use type as an argument. Although, adding an arguments key is more verbose on an already rather verbose object.

{
  "modifiers": [
    {
      "type": "crop",
      "arguments": {
        "left": 12.875,
        "top": 0.125,
        "width": 50,
        "height": 50
      }
    },
    {
      "type": "rotate",
      "arguments": {
        "angle": 90
      }
    }
  ]
}

@ajlende
Copy link
Contributor Author

ajlende commented Oct 6, 2020

oneOf support is being added in WordPress/wordpress-develop#555 which will make the JSON validation for this easier

@ajlende ajlende force-pushed the fix/image-editor-batch-api branch 2 times, most recently from 4185d8c to 30fd274 Compare November 10, 2020 22:01
@ajlende ajlende force-pushed the fix/image-editor-batch-api branch from 30fd274 to c901297 Compare November 10, 2020 22:04
@ajlende
Copy link
Contributor Author

ajlende commented Nov 10, 2020

@TimothyBJacobs I saw that oneOf support got merged, so I updated this with the new schema. How were you planning on maintaining backwards compatibility with the old API? And how should the old one be marked as deprecated?

@TimothyBJacobs
Copy link
Member

I saw that oneOf support got merged, so I updated this with the new schema.

Great! We should make sure to add a title for each oneOf entry.

How were you planning on maintaining backwards compatibility with the old API?

At the beginning of the request it should check for the old request parameters, and then transform them to the new syntax as an array.

And how should the old one be marked as deprecated?

We don't have a mechanism to deprecate parameters in the REST API at the moment. We could potentially add a deprecated keyword to the parameter schema.

@ajlende
Copy link
Contributor Author

ajlende commented Dec 29, 2020

@ajlende ajlende closed this Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants