Mask pattern penalty rules wrong? Or old?

[swifthand]

Hello!

Thank you for this library and for your effort maintaining it for so long!

In using this library, I have recently concluded that three of the four penalty rules for selecting a mask pattern are not correct. Specifically the methods:

• QRUtil::demerit_points_1_same_color
• QRUtil::demerit_points_3_dangerous_patterns
• QRUtil::demerit_points_4_dark_ratio

I have used RQRCode for some time, and so I did not come to this conclusion lightly. I have already fixed these for my own purposes with an undesirable combination of subclassing and monkeypatching. In solving this puzzle, I have many test cases to validate the correctness on various arrangements of modules (manually verified with pen-and-paper), based on the ISO 18004:2015 specification.

Before I put effort into writing up a larger issue and pull request, I wanted to ask whether this is intentional?
For example, are these rules adapted from some earlier version of the ISO standard? I only have access to the 2015 document, and I am aware that this project is older than that.

I do not wish to waste anyone's time.

[whomwah]

Hi Paul @swifthand ,

Sorry its taken a while to see your issue. What you allude to is correct. The Spec that was used to write this lib would have been one from 2008 when the lib was created! and although some PRs in the past may have brought things further up to date is not actively something that has been done.

I actually get very few requests of things not working as expected vs the user install base numbers so changing anything in core is always going to feel like a risk.

So I guess this comes down to:

1. You: how much work you are willing or is required to move things closer to the latest spec
2. Me: what is the risk in changing something that appears to be working for most people. I wouldn't know that until 1.

D

[swifthand]

@whomwah

TL;DR: Viewing changes in a foundational library through the lens of risk is a value I share with you. I will put together a PR in the coming weeks, including tests, and will be as extensive as I can with the explanations.

Expanded thoughts:
In this particular situation, because all eight results of all eight mask patterns are technically valid QR Codes, none of them should fail regardless which is chosen. In that sense, a library could simply choose one via RNG and the codes would still scan just fine most of the time. This just re-prioritizes towards a code that should be less likely to fail in sub-optimal scanning conditions: very oblique angles, distorted surfaces, certain 2D scanners, and so on.

The only reason I even became aware of this, was at $WORK when partnering with another company, we found that our systems did not produce the same QR code for the same data the majority of the time. Our respective chosen codes were still scanning correctly, and we did consider ignoring this. However, there were overriding UX concerns: the end users are retail employees. If they see different codes side-by-side for the same voucher, it might lead them to incorrectly suspect some sort of coupon fraud is afoot, which is a major concern in retail settings. In the case of Version 1 codes, they can appear very obviously different to the naked eye.

This sent me on a dive into not only RQRCode, but also a Java library, a Python library, and two JavaScript libraries. I found minor issues with two of those four, but all of them agreed on the selected mask pattern over 95% of the time, while RQRCode agreed less than half of the time. I need to revisit my notes to be sure, but I recall that one of the four penalty rules in particular explained almost all of the difference in RQRCode.

In terms of how much work this is for me, I already have implemented the 2015-based changes internally to a project. It is just a matter of extracting and refactoring it to be part of this library instead of a wrapper around it.

[whomwah]

@swifthand

Thanks for the details and taking the time to figure all this out and I can certainly see a benefit to fixing the issue. I like to think I'm quite pragmatic, so once a PR is available I'm sure we can move this forward 👍