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

Fail to generate more than one QR with different gradients #186

Closed
j3j5 opened this issue Sep 28, 2024 · 4 comments · Fixed by #188
Closed

Fail to generate more than one QR with different gradients #186

j3j5 opened this issue Sep 28, 2024 · 4 comments · Fixed by #188

Comments

@j3j5
Copy link
Contributor

j3j5 commented Sep 28, 2024

Generating more than one QR with different gradients while using the SVG backend causes all the QRs to render the same gradient as the first QR.

Example code:

$types = ['HORIZONTAL', 'VERTICAL'];
foreach ($types as $type) {
    $gradient = new Gradient(
        new Rgb(0, 0, 0),
        new Rgb(255, 0, 0),
        GradientType::$type()
    );
    $renderer = new ImageRenderer(
        new RendererStyle(size: 400, fill: Fill::withForegroundGradient(
            new Rgb(255, 255, 255), 
            $gradient, 
            EyeFill::inherit(), 
            EyeFill::inherit(), 
            EyeFill::inherit())
        ),
        new SvgImageBackEnd()
    );
    $writer = new Writer($renderer);

    $strings[] = $writer->writeString('Hello World!');
}

Result:
Screenshot_20240928_001125

Expected result:
Screenshot_20240928_001231

The issue comes from the SVGImageBackend class, when generating the gradient, it gives it an id based on the number of gradients present on the same QR, but if more than one QR is on the same page, the id is the same (g1 or whatever the number).

There can be several ways to fix this, the easiest I can think of is to attach a random number to the id to make sure it is unique ($id = sprintf('g%d-' . random_int(PHP_INT_MIN, PHP_INT_MAX), ++$this->gradientCount) ), another way could be creating a hash based on the gradient properties (type, start color and end color) and attach it to the id after the count. Something like:

$hashValue = $this->getColorString($startColor) . $this->getColorString($endColor) . $gradient->getType();
if ($startColor instanceof Alpha) {
    $hashValue .= (string) $startColor->getAlpha();
}
$id = sprintf('g%d-' . hash('xxh128', $hashValue), ++$this->gradientCount);

XXH128 is supposed to be really fast and 8.1 is now the minimum supported version which added support for it.

I'm happy to create a PR in order to fix this, let me know which way you prefer and I'll implement it.

Ps.- Thanks for this great library! 💪

@DASPRiD
Copy link
Member

DASPRiD commented Sep 28, 2024

I think going with random_int might be preferred here, though preferably encoded as hex or even base64 for size reasons (basically a nano ID). Reason which speaks for that over a counting variable:

Consider generating two different QR codes in two different requests but storing them and displaying them again inline – they'd have the same ID if using an instance counter per request.

@j3j5
Copy link
Contributor Author

j3j5 commented Sep 28, 2024

I just did some tests:

hash('xxh64', 'test')
// = "4fdcca5ddb678139"

hash('xxh128', 'test')
// = "6c78e0e3bd51d358d01e758642b85fb8"

dechex(PHP_INT_MAX)
// = "7fffffffffffffff"

base64_encode(PHP_INT_MAX)
// = "OTIyMzM3MjAzNjg1NDc3NTgwNw=="

If you are concerned about size, hex seems to be the way to go (although xxh64 is the same size), base64 and xxh128 are similar in size. The only reason for going with the hash is that we get rid of any (admittedly small) possibility of id collisions (they must be the same gradient to collide, which won't matter anyway), but I agree the chances are really small and it may not be worth it the extra complexity.

As I said, up to you, I'll push PR with your choice, just lmk!

@j3j5
Copy link
Contributor Author

j3j5 commented Sep 28, 2024

Just as a curiosity, but I've done another (unscientific) test to try performance:

// hash xxh64
> $test = ['abc', 'cba', 'test', 'test1', 'abcdefghijklmnopqrstuv']; 
$start = microtime(true); 
foreach(range(1, 4000000) as $i) { 
    hash('xxh64', $test[$i%5]); 
} 
$end = microtime(true); 
echo ($end - $start);
// 0.93287110328674


// hash xxh128
$test = ['abc', 'cba', 'test', 'test1', 'abcdefghijklmnopqrstuv']; 
$start = microtime(true); 
foreach(range(1, 4000000) as $i) { 
    hash('xxh128', $test[$i%5]); 
} 
$end = microtime(true); 
echo ($end - $start);
// 1.3112859725952


// dechex(random_int())
$start = microtime(true); 
foreach(range(1, 4000000) as $i) { 
    dechex(random_int(PHP_INT_MIN, PHP_INT_MAX)); 
} 
$end = microtime(true); 
echo ($end - $start);
// 6.8923921585083

@DASPRiD
Copy link
Member

DASPRiD commented Sep 28, 2024

xxh64 should be decent enough I'd say :)

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 a pull request may close this issue.

2 participants