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

Crop mode does not work? #115

Open
jhannwong opened this issue Apr 23, 2016 · 7 comments
Open

Crop mode does not work? #115

jhannwong opened this issue Apr 23, 2016 · 7 comments
Labels

Comments

@jhannwong
Copy link

The image gets resized instead. Using ImageMagick.

Both width and height are set. aspectRatio is set to false.

@andismith
Copy link
Owner

Hi,

Please can you provide a configuration file?

Thanks,
Andi

@cleverjam
Copy link

cleverjam commented Jun 29, 2016

Hi @andismith

I am currently experiencing the same issue, however I am unsure if this is being caused by my grunt configuration, hopefully it is though.

responsive_images: {
  dev: {
    options: {
      engine: 'im',
      sizes:[
        {
           width: 600,
           height: 300,
           suffix: '',
           quality: 50,
           gravity: 'center',
          aspectRatio: false
       }
 //Files and stuff below

The images are being resized no problem, but no cropping is taking place.

@andismith andismith added the bug label Sep 13, 2016
@jhannwong
Copy link
Author

@andismith Please see my pull request at #124 ?

The code that checks for 'sample' function is wrongly done in an if-else. The default behavior 'resize' should have been specified much higher up, preferably at the start of that code block that parses user-specified task options.

For this PR, I have done a quick fix. Do let me know if you want me to do some refactor to make the code "more correct". Thanks!

@mburazin
Copy link

Hi @andismith,

I have also stumbled into this issue.
After diving into the code and analyzing the pull request #124 I have a few questions.

In the documentation it is specified:

If aspectRatio is set to false and both width and height are specified, the image will be cropped.

Why was this behavior chosen? What happens if we want to really change both the width and the height by not keeping the aspect ratio and without cropping (forced resize).
I'm also asking because I see in the test suite you are also assuming that the resize of the images should be performed in the same scenario without keeping the aspect ratio.
Judging by the way the expected image dimensions are set up, a resize seems to be expected, because some of the image dimensions are being stretched.
Stretching will not be performed in case of cropping. That's why the pull request #124 is failing the test cases.

So, another idea would be then to not mix these two concepts. In other words, maybe we should have a separate option for cropping and not implying it through aspectRatio=false and specified width and height?

I could implement this if agreed.

Appreciate your answer.

Thanks,
//Marko.

@jon-wong-sutd
Copy link

Hi @mburazin

My PR #124 is really a stop-gap measure. Yes, I have also determined that there are problems with the test suites, which causes the correct code at #124 to fail. The error in the test suites is another issue altogether. It seems @andismith is too busy to check his test suites right now.

Coming back to cropping with grunt-responsive-images, I should tell you that I quickly came across other problems even after patching in #124 for my own private fork.

The main issue is that grunt-responsive-images seems written primarily for ImageMagick. ImageMagick sets its own command syntax, which can sometimes be inconsistent (not backwards-compatible, at least). See comparison with ImageMagick here, which mentions "ImageMagick filter arguments producing convolution matrices are now producing lower-order convolution matrices representing perhaps an order of magnitude less work given the same arguments".

Whereas ImageMagick sets and changes its command syntax at its own whims, GraphicsMagick tried to faithfully adhere to ImageMagick's conventions, until it became impossible because ImageMagick deviated from its own conventions.

That said, grunt-responsive-images currently does not allow you to fully use all of ImageMagick's nor GraphicsMagick's full power.

I had planned to rewrite grunt-responsive-images completely, allowing for ordered list of image processing commands. Before I find time to get to that fork, though, I have currently found best mileage with grunt-exec executing GraphicsMagick commands directly (shell command). You can see my Gruntfile here, but please refrain from copying my answers! :-P I can see you are also a Udacity student, and you must be doing the same homework I had done before!

Meantime, please get in touch with Udacity about this glaring bug with grunt-responsive-images. Udacity had kinda ignored me about this for over a year now. I'm a hacker and reverse-engineer, so I could take apart any tool Udacity wants us to use. But not every student likes to hack tools open.

@mburazin
Copy link

Hi @jon-wong-sutd,

Thanks a bunch for the elaboration and sharing your experiences. I will let Udacity know about this bug. In the meantime I can use grunt-exec as you suggested, and won't even look at your code to avoid unconsciously copying your answer :-)

But, just to ask, are you saying that this project (grunt-responsive-images) is or will/should be discontinued? Does it make sense to fix this bug or implement real cropping and continue maintaining/using this project considering all the issues you have mentioned?

I see that it hasn't been updated lately.

Thanks again!

@jon-wong-sutd
Copy link

Hi @mburazin

Command-Line Interface is Complete

In the meantime I can use grunt-exec as you suggested

You can try ImageMagick or GraphicsMagick. For the "art direction" you need in your project, both are a lot more than adequate and convenient. Here's a challenge: I suggest you use ImageMagick, since I used GraphicsMagick. The GraphicsMagick docs (esp. note command convert is just as good as the ImageMagick docs.

Both ImageMagick's and GraphicsMagick's full power can be accessed via command-line, so using grunt-exec will work for you.

Serve More Laymen, or Serve Yourself?

But, just to ask, are you saying that this project (grunt-responsive-images) is or will/should be discontinued? Does it make sense to fix this bug or implement real cropping and continue maintaining/using this project considering all the issues you have mentioned?

Since grunt-exec is more than adequate to access the full power of GraphicsMagick and ImageMagick, I decided that I didn't need to spend time on making the interface more layman-friendly. I had quickly learned the command-line interface for both GraphicsMagick and ImageMagick.

If you wanna serve more Udacity students, who would prefer grunt, I would say you and I fork this project and make it truly access the full power of those 2 tools.

The key addition is ordered list of processing command. It will entail a huge rewrite of this project. Hence, it is nigh impossible to merely correct and maintain this project.

Both tools allow an ordered (sequential) composition of multiple commands. For eg, you may first want to crop an image to an area somewhere top-right, and then rotate the result, and lastly scale the result. (You'll see that rotating first will hinder the crop operation.)

Currently, grunt-responsive-images only allows 1 command to go through. You can either crop or rotate, but not both, and certainly not in any specific order. In cases where it does allow composite operations, it does so inconsistently and often even incorrectly (as seen in this simple cropping bug we encountered).

Also, here's a heads up: plain cropping in grunt-responsive-images, imagining that it works as intended, does not serve your needs in your Udacity project. Note that horses.jpg requires you to crop off-center. You need to specify the location of the crop, not just the size of the crop. As a web developer/designer, art direction is more involved than simply cropping from center.

So yeah, grunt-responsive-images is quite far from being a complete (enough) solution.

Grunt or Gulp?

The question also boils down to grunt vs gulp in terms of convention vs flexibility. Which do you want to use? (If you're familiar with Maven, then it's like Maven vs Gradle.)

Some possibly boring details about convention vs flexibility.

In grunt, the convention is to always use configurable tasks (ie, configurable boilerplates). So, if the provided tasks (eg grunt-responsive-images, grunt-exec, etc) does not allow certain configurations you need, you'll have to:

  1. Hack them open
  2. Spend time understanding the original author's codes (possibly an unconventional mess)
  3. Spend time crafting your corrections/additions into the codes' conventions.

In gulp, the "boilerplate policy" is more flexible in that it only provides a conventional mechanism for your tasks to pass output to other tasks as input (much like shell piping). The downside is you need to be pretty good at Javascript coding to use Gulp.

In short, grunt provides more handholding and possibly reduces human error, whereas gulp provides conciseness (direct Javascript scripting) and power which would scare Javascript newbies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants