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

Port current version of RSSExpandedReader to javascript #630

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

opl-
Copy link
Contributor

@opl- opl- commented Nov 23, 2024

Based on zxing commit 2dfb2054af1e1872ac5354e6d8218931fb88e021.

Finished porting all the code. Incorporated the following fixes: zxing/zxing#1680 zxing/zxing#1692 zxing/zxing#1693 (By manually comparing the existing javascript implementation with the java code).

Didn't attempt running it yet, but all the remaining Java code got ported, including some logic which depended on the values being integers.

Related: #80

@opl-
Copy link
Contributor Author

opl- commented Nov 23, 2024

TODO:

  • Remove "(not production ready!)" from README
  • Remove "RSS Expanded reader IS NOT ready for production yet! use at your own risk."

@opl- opl- force-pushed the feat/rss-expanded-port branch from 5dbc695 to 08500d5 Compare November 25, 2024 12:15
opl- added 3 commits November 25, 2024 13:27
Based on zxing commit 2dfb2054af1e1872ac5354e6d8218931fb88e021.
Happens specifically when the source and destination are the same: System.arraycopy(counters, 0, counters, 1, counters.length - 1)
@opl- opl- force-pushed the feat/rss-expanded-port branch from 08500d5 to e115df4 Compare November 25, 2024 19:22
@opl-
Copy link
Contributor Author

opl- commented Nov 25, 2024

Another batch of changes. Can rebase later. (RSSExpandedReader still can't read anything correctly.)

  • Removed type from catch blocks (} catch (ex: any) { -> } catch (ex) {)
  • Fixed AI01decoder still incorrectly performing pad checks by depending on int division behavior which doesn't exist in JavaScript.
  • Turned startEnd into a tuple type.
  • Ported RSSExpandedInternalTestCase.
  • Fixed missing splice/subList().clear().
  • Correctly implemented removePartialRows() and isPartialRow()
  • Fixed integer divisions returning floats.
  • Fixed System.arraycopy smearing the same value over the entire array (by copying a value into the position it was about to read from).
  • Fixed ExpandedRow.isEquivalent returning false if both arrays are empty (my bad).
  • Fixed ExpandedRow constructor not making a copy of received pairs list.
  • Ported RSSExpandedStackedInternalTestCase.
  • Fixed decoders calling StringBuffer.append with a number, resulting in the value being interpreted as a code point instead of a sequence of numbers.
  • Fixed more hidden integer divisions returning floats (forgot to look for the /= operator).

@opl-
Copy link
Contributor Author

opl- commented Nov 25, 2024

Was missing one integer division and accidentally swapped a condition during minor refactoring.

Anyway, it works. There are still 2 images failing with and without try harder in RSSExpandedBlackBox2, those being 3_06 and 4_02, but I have no energy to even attempt to tackle that.

Not rebasing/squashing in case it makes a review easier. It probably won't.

Edit: As evidenced by the edits, I did end up looking into the failing tests. Both images contain barcodes oriented vertically (or in other words, rotated 90°). I believe rotating the images by 90° should be handled by try harder - not sure why it's not happening.

Edit: SharpImage, which is used as the luminance source in tests, implements rotation functions with no-ops. Implementing those is a pain because sharp's API makes all transformations async, and I don't feel like doing transpositions by hand.

@opl- opl- marked this pull request as ready for review November 25, 2024 21:26
@werthdavid werthdavid self-requested a review December 1, 2024 20:10
@werthdavid werthdavid merged commit 8b5bf58 into zxing-js:master Dec 2, 2024
2 checks passed
@werthdavid
Copy link
Member

Some tests are still failing

@opl-
Copy link
Contributor Author

opl- commented Dec 2, 2024

I assume you're talking about the ones mentioned here. Those try to read barcodes laid out vertically, and so need SharpImageLuminanceSource to correctly implement the rotation functions. I believe SharpImage* is only used for unit tests, so this shouldn't be a problem in normal operation.

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