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

Added scale and resizeProportionally methods. #227

Closed
wants to merge 4 commits into from

Conversation

VVelda
Copy link

@VVelda VVelda commented Jun 12, 2018

I know, there is snippet $image->resize($image->getWidth() * .5) to mimic scale function, but it would be convenient to have some method with semantic.
Also added resizeProportionally method, which is somehow similar to bestFit, but final image have always specified dimensions with some background color.


// fill background, then place centered resized image
$newImage = imagecreatetruecolor($width, $height);
fwrite(STDERR, "test: " . $color['red'] . " " . $color['green'] . " " . $color['blue'] . " " . $color['alpha']);
Copy link
Owner

Choose a reason for hiding this comment

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

This needs to be removed.

$height = $width;

$aspectRatio = $width / $height;
if($aspectRatio > $this->getAspectRatio())
Copy link
Owner

Choose a reason for hiding this comment

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

Conditional formatting is inconsitent. Please use brackets :)

$color = imagecolorallocatealpha(
$newImage, $color['red'], $color['green'], $color['blue'], 127 - $color['alpha']*127
);
imagefill($newImage, 0, 0, $color);
Copy link
Owner

Choose a reason for hiding this comment

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

I think you'd be better off creating a new SimpleImage instance and using overlay() instead of using raw image functions.

@claviska
Copy link
Owner

claviska commented Jun 12, 2018

Thanks for the thorough PR! I marked up a couple things I noticed in the code.

The scale() method is convenient and looks good!

I really think creating a new SimpleImage instance and using overlay() for resizeProportionally() will be better than using raw image methods. For example:

$newImage
  ->fromNew($width, $height, $color)
  ->overlay($originalImage);

The overlay() method works with transparent GIFs and PNGs, so it will prevent some edge cases where transparency gets messed up. (It uses imageCopyMergeAlpha instead of imagecopy to do this.)

}

//
// Resize an image to tightly fit a rectangular of the specified dimensions, without deformation.
Copy link
Owner

Choose a reason for hiding this comment

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

"rectangular" and "centred" typo

@VVelda
Copy link
Author

VVelda commented Jun 13, 2018

Hello! I've did some requested changes.

I do not change requestProportionally implementations as this is working – I have tested transparent img on transparent bg, nontransparent on transparent and transparent on nontransparent. You can verify with example/index_mod.php.

@claviska
Copy link
Owner

The point is to eliminate duplicate code, especially code that’s prone to edge cases. Transparent images in GD are a pain. The overlay method is a battle-tested way to merge two images.

In its current state, when new edge cases come up, we’ll need to fix them in multiple methods. This means extra work and leaves room for error. By using existing methods, we’ll only have to fix those types of things once. 😄

@claviska
Copy link
Owner

claviska commented Aug 7, 2020

Closing this due to lack of activity.

@claviska claviska closed this Aug 7, 2020
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