-
Notifications
You must be signed in to change notification settings - Fork 2
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
Create an advanced image rendering solution #230
Comments
Proposed approach
ConfigurationThe default You'll be able to define your Potential problem with generating the retina image variantLet's say you have an 100x100 image and you do a transformation like this:
In this scenario, the image will be downscale to 50x50. Then the 50x50 will be upscale back to 100x100. Obviously this will lead to a degradation ... which would defeat the entire purpose of this exercise. My suggestion for tackling would like something like this. // This will return the orginal unmanipulated image
$original = $image->getFailover();
$variant = $image->getVariant();
$manipulations = $this->decodeVariants($variant);
foreach ($manipulations as $manipulation) {
// This method would rerun a manipulation, but doubling the dimensions
$original = $this->rerunManipulation($original, $manipulation);
}
$original = $orginal->ResizedImage($image->getWidth(), $image->getHeight()) Some of the image manipulations will change the aspect ratio of the picture, so you can't blindly take the dimension of The "rerun the manipulation" approach should work in most cases ... although there will be scenario where you might get slightly different results e.g. FitMax or ScaleMax. |
Proposed classes/interfaces seem fine, though that's probably more implementation detail than is required here. That said it looks like you've made an assumption that only
What would the PictureSourceList look like? Would this be a list of variants to create and % size they should be in relation to the original?
I'm making the assumption here that the yml config would contain percentages smaller than the original image size. In that scneario you shouldn't get into scenario where you are resizing down then up. Presumbably that's a bad assumption by me? How would you get into this scenario? |
I don't see the benefit of having so many new classes/interfaces... what does the This feels like a lot of abstraction that I feel isn't really necessary. |
Yes, I made the assumption that we would be using If we only cared about retina images, we could get away with only using It probably wouldn't be horribly complicated to allow people to use an
The registration could look something like that: SilverStripe\Core\Injector\Injector:
SilverStripe\ImageSrcSet\ImageSourceGenerator.WebpRetina:
class: SilverStripe\ImageSrcSet\ImageSourceGenerator
constructor:
retina: 2x
format: webp
SilverStripe\ImageSrcSet\ImageSourceGenerator.OriginalRetina:
class: SilverStripe\ImageSrcSet\ImageSourceGenerator
constructor:
retina: 2x
SilverStripe\ImageSrcSet\ImageSourceGenerator.Webp:
class: SilverStripe\ImageSrcSet\ImageSourceGenerator
constructor:
format: webp
SilverStripe\ImageSrcSet\PictureSourceList:
class: SilverStripe\ImageSrcSet\PictureSourceList
constructor:
generators:
- '$%SilverStripe\ImageSrcSet\ImageSourceGenerator.WebpRetina'
- '$%SilverStripe\ImageSrcSet\ImageSourceGenerator.OriginalRetina'
- '$%SilverStripe\ImageSrcSet\ImageSourceGenerator.Webp' The example above assumes that a generic
That example was there to illustrate a potential problem if we just blindly resize whatever image we get from the WYSIWIG. My hope is that developers using this module will only need to provide the generic image they want to use as the fallback. e.g. I want them to be able to write The actual image that will be provided to
It would allow us to be explicit about the attribute that are valid on ImageSource and save us the trouble of constantly having to do empty check on attributes. You could also add some convenience method on it or a I think the small added complexity of having a dedicated class for this will be outweighed by reduce complexity elsewhere. My gut feeling is that having a dedicated class for this would be more elegant. But it probably doesn't matter all that much in the grander scheme of things.
I'm writing this with the expectation that it will be merged back into core at some stage. I'm also trying to leave open the possibility that people could want to use this for other use cases Given the context I'm writing this in, I'm admittedly trying to make the implementation a bit more sophisticated than it needs to be. If we feel the thing is over engineered down the track, we can look at pulling it back a bit to simplify the codebase. |
Just dropping in to mention that we had a need for this a while back, and I ended up building a module that sounds like it has quite a few similarities to the proposed class structure above: https://github.com/kinglozzer/silverstripe-picture. I realise it achieves quite different goals to this ticket (it’s built so image sizes are stored in YAML, just calling If you want to take anything from the module then feel free, otherwise ignore me and sorry for the noise 🙂 |
@kinglozzer Thanks! It's always nice to have some prior art to compare. I'll have a look. They are a few things we added in 5.2 and are planning to add in 5.3 that open new options.
We might be able to adapt your approach or part of it into our own solution. |
Features
ArchitectureI think the approach @kinglozzer is taking is broadly similar to how I was planning to do it.
Some of the differences
Generally speaking, I like @kinglozzer naming convention better, so I'll steal that as well. |
I think a question worth asking at this stage is: Is it worth doing this as a new separate module, or could this functionality be suitably implemented as a PR to Loz's existing module? I don't have an opinion on that, just seems worth asking. |
Very good point @GuySartorelli. I do have an opinion on that :D Adopting Loz's module and bringing it under silverstripe org, or just contributing to it perhaps as co-maintainers, could also alleviate the already voiced concerns of ss just taking ideas and doing their own module for nearly the same functionality vs contributing back to existing community modules (the most recent example being the linkfield module). With the linkfield module I questioned why it was done in the first place, with this image module I see there's some potential to fill a gap in functionality around images in default CMS installs, so I would have less concern in this case, though knowing now there's a module that does nearly all of what we need, some of it better, I'd say we should make a really good argument on why not to contribute there vs doing nearly the same twice. |
For context, this idea was prioritise a bit out of left field. I'm happy to go over the exact reason at our next core committer catch up. To the extent that @kinglozzer would be happy to consider a PR to implement some of the extra feature in this issues' ACs, I'm happy to target a PR there. |
I think it’s worth clarifying what the ultimate goal of this work is - I’ve probably added a lot of scope creep by linking to my module 😅
I think the initial AC was more like 1 or 2, which I think should live in the Silverstripe org/namespace. Basic image rendering is very core to a CMS, and most - if not all - sites will want auto-webp/avif etc, so I think it’d look a bit odd to recommend installing a third-party module (even if it’s officially supported) for that. If we want to offer something like 3, it probably makes sense as a module (i.e. separate from silverstripe-assets). Regardless of whether mine is used as a starting point/inspiration, I imagine it will end up looking quite different anyway. In terms of “ownership” of that theoretical module, my personal preference would be for it to live in the Silverstripe org/namespace - as Silverstripe would be doing a lot of build work, then taking on the maintenance burden, it’d feel weird for it to be all sitting under my nickname. I could either transfer the repo (we’ve done that before), or something new could just be built from scratch taking whatever ideas are useful (which might be the easier approach for prototyping ideas). |
This issue is to implement optional functionality that won't sit inside the commercial support commitments. There's no expectation that Silverstripe will be doing any maintenance of it post-implementation at this stage, I think. |
The idea of creating a new module was to have a proof-of-concept that people could choose to install. This is mostly meant to be a tool to allow us to iterate over quickly and try things. But the long term intention is to merge this kind of functionality back into a core module, probably around CMS 6. I'm making a few minimal change to CMS 5.3 to make it easier for that module to exists: silverstripe/silverstripe-assets#596 I'm somewhat indifferent to whether the proof-of-concept part is in If no one object, I'll start working of |
Why does the CMS squad even take this on then? |
Some of the recent work like image lazy loading and allowing variants with different extension and image converters have unlock some cool new possibilities. But we're not quite sure how best to seize those opportunities. So we want to create a "proof-of-concept" module that people can try out on existing or new projects. This will allow to validate concerns like:
That's where we can to have a proof-of-concept module to validate how these concern will behave in the real world. Once that gets sorted, we'll probably take the lessons we've learn and will probably merge this functionality back into core with some migration path for people who were using the proof-of-concept module. We had a SPIKE internally about this 2-3 years ago. For some reason I can't recall, we kept this in-house instead of discussing it publicly. That's the discussion we had back then. |
Got a draft PR. It meets most of the ACs but still need to be cleaned up a bit. |
Linked PRs have been merged, except for the one to silverstripe-picture though that PR should be dealt with seperately |
Overview
Silverstripe CMS doesn't have a native solution to managing more advanced image use cases such as displaying retina images or rendering images in multiple formats with a fallback.
Recent work has made it easier to convert images between formats (e.g. JPEG to WEBP). We also did some exploratory work following our image lazy loading work a few years ago.
There is an opportunity to explore what modern image rendering could look like in the CMS by providing an MVP solution.
User story
As a developer, I want a transparent way to adopt modern image practices so my end users can benefit from the latest browser standards without being left behind if they use older browsers.
Acceptance criteria
<picture>
tag are highlighted.Possible follow up work
Original cards
Pull requests
The text was updated successfully, but these errors were encountered: