-
-
Notifications
You must be signed in to change notification settings - Fork 223
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
FEATURE: Add focal-point support for ImageVariants (crops) and Thumbnails #5127
Draft
mficzel
wants to merge
4
commits into
neos:8.4
Choose a base branch
from
mficzel:feature/imageFocalPoints
base: 8.4
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mficzel
force-pushed
the
feature/imageFocalPoints
branch
2 times, most recently
from
June 5, 2024 07:32
c9b88f8
to
0cb0548
Compare
mficzel
force-pushed
the
feature/imageFocalPoints
branch
2 times, most recently
from
June 5, 2024 08:37
f5cba4d
to
e63bfeb
Compare
mficzel
changed the title
WIP : FEATRUE: Image focal points
WIP : FEATURE: Image focal points
Jun 5, 2024
mficzel
force-pushed
the
feature/imageFocalPoints
branch
5 times, most recently
from
June 5, 2024 10:43
2bbe2f2
to
1efd301
Compare
mficzel
force-pushed
the
feature/imageFocalPoints
branch
4 times, most recently
from
July 25, 2024 12:47
6bd01c1
to
0a4e5cb
Compare
mficzel
changed the title
WIP : FEATURE: Image focal points
FEATURE: Image focal points
Jul 25, 2024
mficzel
changed the title
FEATURE: Image focal points
FEATURE: Add focal-point support for ImageVariants (crops) and Thumbnails
Jul 25, 2024
mficzel
force-pushed
the
feature/imageFocalPoints
branch
2 times, most recently
from
July 25, 2024 14:25
4421359
to
a59b1ca
Compare
mficzel
force-pushed
the
feature/imageFocalPoints
branch
from
July 26, 2024 08:45
a59b1ca
to
bdb28e1
Compare
mficzel
force-pushed
the
feature/imageFocalPoints
branch
from
July 26, 2024 08:52
bdb28e1
to
17eac5d
Compare
close and reopen to reset ci |
This at least prooves that the tests are running for php 8.2 and 8.3. The failing Stan Tests correctly identifies that this core required php 8.2. After #5188 is fixed this branch should be rebased on 8.4 and should become green. |
Converted to draft until 8.4 is green again and this can be rebased |
…riants No actual calculation yet
…perThingy This obviously will need a better name once we better understand the tasks it will have to perform.
…th focalPoint The preliminary crop ensures: 1. The target image is as large as possible inside the original image dimensions 2. The focal point is as central as possible inside the generated image
mficzel
force-pushed
the
feature/imageFocalPoints
branch
from
August 27, 2024 13:43
17eac5d
to
4265151
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In many cases the crops taken from the center of images are not optimal and important parts of an image get lost. This pr allows to specify a focal-point for imageVariants that is later on respected when crops of the imageVariant are generated.
The implementation uses an preliminary crop for images with a focalPoint that ensures:
In addition this change calculates the focal-point and image dimensions for async thumbnails on generation. That way async thumbnails can be rendered with full width and height informations which allows to make use of those informations in the rendering to prevent layout shifts or to specify the center of an object-fit css rule.
Resolves: #2296
Required Neos.UI Changes neos/neos-ui#3835
focalPointX
?int andfocalPointY
?int of the new ImageVariant that is createdvia endpoint
neos/content/create-image-variant
which points to\Neos\Neos\Controller\Backend\ContentController::createImageVariantAction
Review instructions
!!! By intention the focal point is not added to images in the media library (yet). The main reason for that is that this would require a mechanism to re-render all thumbnails of an asset if the focal point is changed. Also i personally see more value to specify the focal point by editors for a specific crop that is why the implementation cover ImageVariants and Thumbnails. If we see value in focal point support via media library we can add this in future !!!
The main part of this solution is that the calculation of the cropped dimensions was moved out from the
ResizeImageAdjustment
to a newResizeDimensionCalculator
where it can be accessed from other code like theImageThumbnailGenerator
and theThumbnailService
that need to know about image dimensions beforehand. The tests for the now deprecated methods inResizeImageAdjustment
are still in place to prove that the calculations are the same.For now the generated thumbnails will get a small green circle rendered on the focal-point if a focal point was set. This allows to visually verify the results in a human understandable way. After approval this will be removed.
Also the tests with php 8.0 and 8.1 are failing since this will be rebased on to 8.4 once this branch is green again. I used 8.2 features that will not work with lower php versions.
Final Todos before merge - after review and general approval
Checklist
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions