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

Support different extend modes using "extend.extend" config item #3556

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

janaz
Copy link
Contributor

@janaz janaz commented Feb 10, 2023

This is my attempt at implementing the feature asked for in #3534

@lovell suggested the following API for it:

// PROPOSED API, NOT YET AVAILABLE
sharp.extend({
  ...
  extend: 'copy' | 'mirror' | 'repeat'
})
// PROPOSED API, NOT YET AVAILABLE

This PR adds a new config item to sharp.extend as specified in the proposal. The supported values for extend.extend are:

  • background (colour set from the background property) - this is the current behaviour (default)
    extend-equal-background

  • copy (copy the image edges)
    extend-equal-copy

  • repeat (repeat the whole image)
    extend-equal-repeat

  • mirror (mirror the whole image)
    extend-equal-mirror

Supported modes are "background", "copy", "mirror", "repeat" which map
directly to libvips options
@lovell
Copy link
Owner

lovell commented Feb 14, 2023

Hi Tomasz, good to hear from you, thank you very much for this.

It's interesting that you chose ExtendMode as the type name, perhaps we should use this in the API instead?

// PROPOSED API, NOT YET AVAILABLE
sharp.extend({
  ...
  mode: 'copy' | 'mirror' | 'repeat'
})

I generally prefer to map to libvips naming, but we've already deviated with extend already and if "mode" is more natural then that might be better.

EDIT: After a little more thought, I'm favouring extendWith for this as it should hopefully avoid confusion. It was the title of your PR that alerted me to the possible extend.extend conundrum in the first place, so thanks for that.

// PROPOSED API, NOT YET AVAILABLE
sharp.extend({
  ...
  extendWith: 'background' | 'copy' | 'mirror' | 'repeat'
})

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

This is brilliant, thank you again Tomasz. I've left a couple of comments inline and I've given the public API some more thought too with one small change, but otherwise this looks great and is almost ready to go.

src/pipeline.cc Outdated Show resolved Hide resolved
lib/resize.js Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
@janaz
Copy link
Contributor Author

janaz commented Feb 15, 2023

I pushed a commit with all the requested changes. Switching to sharp::AttrAsEnum<VipsExtend> made the code a lot simpler!

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

One small error but otherwise this is good to go, thank you.

src/operations.cc Outdated Show resolved Hide resolved
@lovell lovell merged commit 6f0e6f2 into lovell:flow Feb 17, 2023
@lovell
Copy link
Owner

lovell commented Feb 17, 2023

Thanks again, this will be in v0.32.0.

@lovell lovell added this to the v0.32.0 milestone Feb 17, 2023
lovell added a commit that referenced this pull request Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants