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

readFromFile/readFromBlob result returning also the QR code coordinates inside the source image #248

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

pedrospinto
Copy link

Proposed changes

...as discussed in
#240

Types of changes

What types of changes does your code introduce?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have checked to ensure there aren't other open Issues or Pull Requests for the same update/change
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules
  • Lint and unit tests pass locally with my changes

@codemasher
Copy link
Member

codemasher commented Mar 1, 2024

Hey! Thank you for the PR! I'm going to add some thoughts to the files - no worries, I don't bite :)

2 things kept me thinking:

  • What's the best way to test it? (maybe take one or more of the samples in /tests, determine & verify the results and hardcode them?)
  • How does the ZXing app (from which the decoder is ported) do it? The app displays the centers of the detected finde- and alignment patterns while scanning, so there has to be code that does exactly what this PR is supposed to do.

public int $topRightX;
public int $topRightY;
public int $bottomLeftX;
public int $bottomLeftY;
Copy link
Member

Choose a reason for hiding this comment

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

We don't do public properties here :)

I think we can hand down the whole result from the finder pattern finder as the objects hold other useful values such as the estimated module size, so just add one property (with a getter) that holds the result array private array|null $finderPatterns = null (nullable in case it gets called from the getter before Detector::detect() was called).

$this->topRightY = (int)floor($topRight->getY());
$this->bottomLeftX = (int)floor($bottomLeft->getX());
$this->bottomLeftY = (int)ceil($bottomLeft->getY());

Copy link
Member

Choose a reason for hiding this comment

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

Here just assign the result from the FinderPatternFinder call to the property and unpack the list from that property afterwards:

$this->finderPatterns = (new FinderPatternFinder($this->matrix))->find();
[$bottomLeft, $topLeft, $topRight] = $this->finderPatterns;

private int $topRightX;
private int $topRightY;
private int $bottomLeftX;
private int $bottomLeftY;
Copy link
Member

@codemasher codemasher Mar 1, 2024

Choose a reason for hiding this comment

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

I'd just add 3 properties with the FinderPattern type here. I'm not sure yet what is returned when a pattern is not found - when in doubt, make the fields nullable. (edit: it seems like it will always return 3 instances)

$this->topRightX = $detector->topRightX;
$this->topRightY = $detector->topRightY;
$this->bottomLeftX = $detector->bottomLeftX;
$this->bottomLeftY = $detector->bottomLeftY;
Copy link
Member

@codemasher codemasher Mar 1, 2024

Choose a reason for hiding this comment

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

Here I'd just assign the Detector instance to a property and call $this->detector->getFinderPatterns() only right before assigning the variables to the result object.

Further i think we could add a getCoordinates() method to the ResultPoint class, which does the floor()-ing and all

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