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

Configurable class for DimensionCalculator #204

Closed
wants to merge 2 commits into from

Conversation

bengs
Copy link
Contributor

@bengs bengs commented Feb 8, 2023

Hi, for some reason I've not been able to override DimensionCalculator::class using the example in the recently merged pull request #193 in my AppServiceProvider.php file (maybe the package boot binding order prevents this?) so this pull request changes its binding to use a configurable class, similar to how Spatie Media Library does it.
I was then able to override it with a custom class configurable in config/responsive-images.php eg:

    /*
    |--------------------------------------------------------------------------
    | Dimension Calculator Class
    |--------------------------------------------------------------------------
    |
    | The class that will be used for the breakpoint srcset calculations
    |
    */

    'dimension_calculator' => \App\Media\MyDimensionCalculator::class,

@ncla
Copy link
Collaborator

ncla commented Feb 8, 2023

I am interested what went wrong for you. Could you please share your code on how you tried to bind your implementation (Service Provider and DimensionCalculator class)?

@bengs
Copy link
Contributor Author

bengs commented Feb 8, 2023

Sure here's a public repo I just created you can use for testing using a clean Statamic Starter Kit Multisimplicity install. I then copied the code from your pull request #193 in to AppServiceProvider.php and tested with returning null at the top of calculateForBreakpoint() - it doesn't seem to have any effect on the image srcset outputs if you visit the /rooms page. Did I override it it incorrectly?

/**
 * Bootstrap any application services.
 *
 * @return void
 */
public function boot()
{
      $this->app->bind(DimensionCalculator::class, function () {
          return new class extends ResponsiveDimensionCalculator {
              public function calculateForBreakpoint(Source $source): Collection
              {
                  // return anything here it doesn't seem to have any effect eg return null;
      
                  // Disable JPG sources
                  if ($source->getFormat() === 'original') {
                      return collect([]);
                  }
      
                  // Create widths from 100 to 2000 in 100px increments with fixedHeight passed from params
                  return collect(range(100, 2000, 100))
                      ->map(function ($width) use ($source) {
                          return new Dimensions($width, $source->breakpoint->params['fixedHeight']);
                      });
              }
          };
      });
}

@bengs
Copy link
Contributor Author

bengs commented Feb 8, 2023

In my private app (using this pull request with configurable class) I copied the ResponsiveDimensionCalculator class to App\Media and modified the calculateDimensions() and finishedCalculating() methods to use 2 new config variables:

'predicted_filesize' => 0.4,
'min_srcset_width' => 300,

\App\Media\ResponsiveDimensionCalculator.php

<?php

namespace App\Media;

use Illuminate\Support\Collection;
use Spatie\ResponsiveImages\Source;
use Statamic\Contracts\Assets\Asset;
use Spatie\ResponsiveImages\Breakpoint;
use Spatie\ResponsiveImages\Dimensions;
use Spatie\ResponsiveImages\DimensionCalculator;

/**
 * The original, file-size, aspect-ratio based dimension calculator.
 */
class ResponsiveDimensionCalculator implements DimensionCalculator
{
    public function calculateForBreakpoint(Source $source): Collection
    {
        $asset = $source->breakpoint->asset;
        $width = $asset->width();
        $height = $asset->height();
        $fileSize = $asset->size();

        $ratio = $this->breakpointRatio($asset, $source->breakpoint);
        $glideParams = $source->breakpoint->getImageManipulationParams();

        return $this
            ->calculateDimensions($fileSize, $width, $height, $ratio)
            ->sort()
            // Filter out widths by max width
            ->when((isset($glideParams['width']) || config('statamic.responsive-images.max_width') !== null), function ($dimensions) use ($glideParams, $ratio) {
                $maxWidth = $glideParams['width'] ?? config('statamic.responsive-images.max_width');

                $filtered = $dimensions->filter(function (Dimensions $dimensions) use ($maxWidth) {
                    return $dimensions->getWidth() <= $maxWidth;
                });

                // We want at least one width to be returned
                if (! $filtered->count()) {
                    $filtered = collect([
                        new Dimensions($maxWidth, round($maxWidth / $ratio)),
                    ]);
                }

                return $filtered;
            });
    }

    public function calculateForImgTag(Breakpoint $breakpoint): Dimensions
    {
        $maxWidth = ($breakpoint->parameters['glide:width'] ?? config('statamic.responsive-images.max_width') ?? null);

        $ratio = $this->breakpointRatio($breakpoint->asset, $breakpoint);

        $width = $maxWidth ?? $breakpoint->asset->width();

        return new Dimensions($width, round($width / $ratio));
    }

    public function calculateForPlaceholder(Breakpoint $breakpoint): Dimensions
    {
        return new Dimensions(32, 32 / $this->breakpointRatio($breakpoint->asset, $breakpoint));
    }

    public function breakpointRatio(Asset $asset, Breakpoint $breakpoint): float
    {
        return $breakpoint->parameters['ratio'] ?? ($asset->width() / $asset->height());
    }

    private function calculateDimensions(int $assetFilesize, int $assetWidth, int $assetHeight, $ratio): Collection
    {
        $dimensions = collect();

        $dimensions->push(new Dimensions($assetWidth, round($assetWidth / $ratio)));

        // For filesize calculations
        $ratioForFilesize = $assetHeight / $assetWidth;
        $area = $assetHeight * $assetWidth;

        $predictedFileSize = $assetFilesize;
        $pixelPrice = $predictedFileSize / $area;

        while (true) {
            // $predictedFileSize *= 0.7;
            $predictedFileSize *= config('statamic.responsive-images.predicted_filesize', 0.7);

            $newWidth = (int) floor(sqrt(($predictedFileSize / $pixelPrice) / $ratioForFilesize));

            if ($this->finishedCalculating($predictedFileSize, $newWidth)) {
                return $dimensions;
            }

            $dimensions->push(new Dimensions($newWidth, round($newWidth / $ratio)));
        }
    }

    private function finishedCalculating(float $predictedFileSize, int $newWidth): bool
    {
        if ($newWidth < config('statamic.responsive-images.min_srcset_width', 20)) {
            return true;
        }

        if ($predictedFileSize < (1024 * 10)) {
            return true;
        }

        return false;
    }
}

@ncla
Copy link
Collaborator

ncla commented Feb 8, 2023

In AppServiceProvider you had not imported DimensionCalculator through use statement.

use Spatie\ResponsiveImages\DimensionCalculator;

This will then reveal that you cannot return null value, it must be Collection, even if it is empty.

This works and the end result I see is that there are no <source> tags in the <picture>.

@ncla
Copy link
Collaborator

ncla commented Feb 8, 2023

I am gonna for now close this PR however. You should be able bind it now with success. I do not feel that confident yet to publicly document this feature, including in a config file, if that makes sense (even if I documented this feature in CHANGELOG.. 😅 ). I definitely have thought about this when I was making the PR so maybe in the future.

But it is nice however to see that it is already being used and is found useful.

@ncla ncla closed this Feb 8, 2023
@bengs
Copy link
Contributor Author

bengs commented Feb 8, 2023

Ah sorry VS Code code usually highlights missing imports but didn't for that one 🤦 (I guess because inside bind it wasn't throwing an error). I was intentionally trying to cause an error with return null in the demo (instead of collect) to detect if if the binding was being run. Yes I'm totally fine with binding it like your example - thanks for your help and the new feature it's great - in a big multisite app I wanted to reduce the number of srcsets and your extensible calculator allowed me to do that.

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