Skip to content

Add focal point cropping #48

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.

Sign up for GitHub

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
TorstenDittmann wants to merge 1 commit into from
Closed

Add focal point cropping #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 62 additions & 39 deletions src/Image/Image.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,28 @@ public static function getGravityTypes(): array
/**
* @throws \Throwable
*/
public function crop(int $width, int $height, string $gravity = Image::GRAVITY_CENTER): self
public function crop(
int $width,
int $height,
string $gravity = Image::GRAVITY_CENTER,
?float $x = null,
?float $y = null
): self
{
if (($x === null) !== ($y === null)) {
throw new \InvalidArgumentException('Both focal point coordinates are required');
}

if ($x !== null && ($x < 0 || $x > 1 || $y < 0 || $y > 1)) {

@greptile-apps greptile-apps Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 NaN Bypasses Coordinate Validation

The public float parameters accept NAN, but comparisons such as NAN < 0 and NAN > 1 are false in PHP. This lets a non-finite coordinate reach the crop-offset calculation and produce an unintended crop instead of the expected InvalidArgumentException. Reject non-finite coordinates before checking their range.

Suggested change
if ($x !== null && ($x < 0 || $x > 1 || $y < 0 || $y > 1)) {
if ($x !== null && (!\is_finite($x) || !\is_finite($y) || $x < 0 || $x > 1 || $y < 0 || $y > 1)) {
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Image/Image.php
Line: 114

Comment:
**NaN Bypasses Coordinate Validation**

The public float parameters accept `NAN`, but comparisons such as `NAN < 0` and `NAN > 1` are false in PHP. This lets a non-finite coordinate reach the crop-offset calculation and produce an unintended crop instead of the expected `InvalidArgumentException`. Reject non-finite coordinates before checking their range.

```suggestion
        if ($x !== null && (!\is_finite($x) || !\is_finite($y) || $x < 0 || $x > 1 || $y < 0 || $y > 1)) {
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject non-finite focal coordinates

When either coordinate is NAN, all four range comparisons evaluate to false, so the value passes validation despite not being between 0 and 1. The subsequent min/max calculation can silently select an image boundary (or produce conversion warnings depending on the PHP version), yielding an incorrect crop; explicitly reject non-finite coordinates before calculating the offsets.

Useful? React with 👍 / 👎.

throw new \InvalidArgumentException('Focal point coordinates must be between 0 and 1');
}

$hasFocalPoint = $x !== null;
$focalX = $x;
$focalY = $y;

// if no changes to Gravity, Width or Height, don't process image
if ($gravity === Image::GRAVITY_CENTER
if ($gravity === Image::GRAVITY_CENTER && !$hasFocalPoint
&& (
($width !== 0 && $height !== 0)
&& ($width === $this->width && $height === $this->height)
Expand All @@ -127,7 +145,7 @@ public function crop(int $width, int $height, string $gravity = Image::GRAVITY_C

$resizeWidth = $this->width;
$resizeHeight = $this->height;
if ($gravity !== Image::GRAVITY_CENTER) {
if ($gravity !== Image::GRAVITY_CENTER || $hasFocalPoint) {
$targetAspect = $width / $height;
if ($targetAspect > $originalAspect) {
$resizeWidth = $width;
Expand All @@ -139,40 +157,45 @@ public function crop(int $width, int $height, string $gravity = Image::GRAVITY_C
}

$x = $y = 0;
switch ($gravity) {
case self::GRAVITY_TOP_LEFT:
$x = 0;
$y = 0;
break;
case self::GRAVITY_TOP:
$x = ($resizeWidth / 2) - ($width / 2);
break;
case self::GRAVITY_TOP_RIGHT:
$x = $resizeWidth - $width;
break;
case self::GRAVITY_LEFT:
$y = ($resizeHeight / 2) - ($height / 2);
break;
case self::GRAVITY_RIGHT:
$x = $resizeWidth - $width;
$y = ($resizeHeight / 2) - ($height / 2);
break;
case self::GRAVITY_BOTTOM_LEFT:
$x = 0;
$y = $resizeHeight - $height;
break;
case self::GRAVITY_BOTTOM:
$x = ($resizeWidth / 2) - ($width / 2);
$y = $resizeHeight - $height;
break;
case self::GRAVITY_BOTTOM_RIGHT:
$x = $resizeWidth - $width;
$y = $resizeHeight - $height;
break;
default:
$x = ($resizeWidth / 2) - ($width / 2);
$y = ($resizeHeight / 2) - ($height / 2);
break;
if ($hasFocalPoint) {
$x = \max(0, \min($resizeWidth - $width, $focalX * $resizeWidth - $width / 2));
$y = \max(0, \min($resizeHeight - $height, $focalY * $resizeHeight - $height / 2));
} else {
switch ($gravity) {
case self::GRAVITY_TOP_LEFT:
$x = 0;
$y = 0;
break;
case self::GRAVITY_TOP:
$x = ($resizeWidth / 2) - ($width / 2);
break;
case self::GRAVITY_TOP_RIGHT:
$x = $resizeWidth - $width;
break;
case self::GRAVITY_LEFT:
$y = ($resizeHeight / 2) - ($height / 2);
break;
case self::GRAVITY_RIGHT:
$x = $resizeWidth - $width;
$y = ($resizeHeight / 2) - ($height / 2);
break;
case self::GRAVITY_BOTTOM_LEFT:
$x = 0;
$y = $resizeHeight - $height;
break;
case self::GRAVITY_BOTTOM:
$x = ($resizeWidth / 2) - ($width / 2);
$y = $resizeHeight - $height;
break;
case self::GRAVITY_BOTTOM_RIGHT:
$x = $resizeWidth - $width;
$y = $resizeHeight - $height;
break;
default:
$x = ($resizeWidth / 2) - ($width / 2);
$y = ($resizeHeight / 2) - ($height / 2);
break;
}
}
$x = \intval($x);
$y = \intval($y);
Expand All @@ -181,7 +204,7 @@ public function crop(int $width, int $height, string $gravity = Image::GRAVITY_C
$this->image = $this->image->coalesceImages();

foreach ($this->image as $frame) {
if ($gravity === self::GRAVITY_CENTER) {
if ($gravity === self::GRAVITY_CENTER && !$hasFocalPoint) {
$frame->cropThumbnailImage($width, $height);
} else {
$frame->scaleImage($resizeWidth, $resizeHeight, false);
Expand All @@ -191,7 +214,7 @@ public function crop(int $width, int $height, string $gravity = Image::GRAVITY_C

$frame->setImagePage($width, $height, 0, 0);
}
} elseif ($gravity === self::GRAVITY_CENTER) {
} elseif ($gravity === self::GRAVITY_CENTER && !$hasFocalPoint) {
$this->image->cropThumbnailImage($width, $height);
} else {
$this->image->scaleImage($resizeWidth, $resizeHeight, false);
Expand Down
38 changes: 38 additions & 0 deletions tests/Image/ImageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,44 @@ public function testCrop100x100(): void
unlink($target);
}

public function testCropFocalUsesNormalizedCoordinates(): void
{
$source = new \Imagick();
$source->newImage(6, 2, 'red', 'png');
$draw = new \ImagickDraw();
$draw->setFillColor('green');
$draw->rectangle(2, 0, 3, 1);
$draw->setFillColor('blue');
$draw->rectangle(4, 0, 5, 1);
$source->drawImage($draw);

$image = new Image($source->getImageBlob());
$image->crop(2, 2, x: 0.75, y: 0.5);

@greptile-apps greptile-apps Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Animated Focal Path Untested

The successful focal-point test covers only a single-frame PNG, while focal coordinates send animated images through a separate per-frame scale-and-crop path. Add an animated focal-point test that verifies frame dimensions, selected content, and aggregate delay so regressions in this newly reachable path are detected.

Knowledge Base Used:

Prompt To Fix With AI 
This is a comment left during a code review.
Path: tests/Image/ImageTest.php
Line: 126

Comment:
**Animated Focal Path Untested**

The successful focal-point test covers only a single-frame PNG, while focal coordinates send animated images through a separate per-frame scale-and-crop path. Add an animated focal-point test that verifies frame dimensions, selected content, and aggregate delay so regressions in this newly reachable path are detected.

**Knowledge Base Used:**
- [Image manipulation library](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/image/-/docs/image-manipulation-library.md)
- [Animated image processing](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/image/-/docs/animated-image-processing.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex


$result = new \Imagick();
$result->readImageBlob($image->output('png', 100) ?: '');
$color = $result->getImagePixelColor(1, 1)->getColor();

$this->assertGreaterThan($color['r'], $color['b']);
$this->assertGreaterThan($color['g'], $color['b']);
}

public function testCropFocalRejectsCoordinatesOutsideTheImage(): void
{
$image = new Image(file_get_contents(__DIR__ . '/../resources/disk-a/kitten-1.jpg') ?: '');

$this->expectException(\InvalidArgumentException::class);
$image->crop(100, 100, x: 1.1, y: 0.5);
}

public function testCropFocalRequiresBothCoordinates(): void
{
$image = new Image(file_get_contents(__DIR__ . '/../resources/disk-a/kitten-1.jpg') ?: '');

$this->expectException(\InvalidArgumentException::class);
$image->crop(100, 100, x: 0.5);
}

public function testCropGravityNw(): void
{
$image = new Image(file_get_contents(__DIR__ . '/../resources/disk-a/kitten-1.jpg') ?: '');
Expand Down