Skip to content

Issue 709 #712

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

Closed
wants to merge 6 commits into from
Closed

Issue 709 #712

wants to merge 6 commits into from

Conversation

lashus
Copy link
Contributor

@lashus lashus commented May 18, 2019

I've changed the cropping behaviour as written in #709. Not sure though how to test it and also without using distort and SRT we are changing the image size (in GD that doesnt happen). Not sure how to refactor it other way though as with using image distortion with SRT it's hard to rotate it in same way (using same point).

@ausi
Copy link
Collaborator

ausi commented Jul 6, 2019

Not sure though how to test it

You can add a test like the following to the AbstractImageTest class:

public function testRotateWithCrop()
{
    $palette = new RGB();
    $color = $this
        ->getImagine()
        ->create(new Box(100, 100), $palette->color('#f00'))
        ->rotate(45, $palette->color('#fff'))
        ->crop(new Point(0, 0), new Box(100, 100))
        ->getColorAt(new Point(0, 50));
    $this->assertSame('#ffffff', (string) $color);
}

It creates a red 100x100 square image rotates it by 45 degrees and crops it to 100x100. After that the center pixel on the left side should be white as you can see in the following example images:

wrong: wrong correct: correct

we are changing the image size (in GD that doesnt happen)

I don’t understand what you mean by that. I tested all three drivers and (with the fix for Imagick) they all behave the same. GD also increases the image size if you rotate the image by 45 degrees.


Your pull request seems to include unrelated commits. I think you meant to include just the commit 58193ee correct?

@ausi
Copy link
Collaborator

ausi commented Jan 15, 2020

I created a followup pull request that includes the changes from my comments: #734

@mlocati
Copy link
Collaborator

mlocati commented Nov 3, 2020

Superseded by #734
Please don't try to fix multiple stuff in the same pull request

@mlocati mlocati closed this Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants