Added option to retrieve the colors used in the QR code from the image. #76

david-boles · 2018-07-13T19:49:11Z

Implementation for #73.

…mendations from cozmo

cozmo · 2018-07-18T22:52:41Z

Awesome work on the PR. It's been a pretty busy week for me, but I'll give it a look soon.

Meanwhile in #77 I've added the options object for another option we needed to add, so you may be able to leverage some of the work there.

cozmo · 2018-07-19T04:20:51Z

If you want faster iteration on this lint stuff you can just run npm run-script lint locally.

david-boles · 2018-07-19T04:27:05Z

That probably would have been a good idea :)

david-boles · 2018-07-19T04:34:33Z

@cozmo It looks like the tests are failing due to how the objects are getting serialized? Not sure how to fix that...

david-boles · 2018-07-19T20:44:24Z

Yeah, I'm not sure how to fix that. it seems to be caused by data type issues...

cozmo

Hey! Thanks for submitting this, and sorry for the slight delay in review, been a busy week.

In general things look pretty good, I left some small comments on a few things.

As for testing, my thinking is that instead of modifying the end to end tests (which mostly serve to make sure changes don't drastically decrease the average success rate) I'd add a few specific hand crafted tests to src/color-retriever/test.ts - In the style of decoder. Take a few images and write descriptive test cases, feeding them into and out of the retrieve color function. This allows us to test retrieve colors carefully, where as the end to end tests are pretty noisy so probably wouldn't serve to catch any bugs/changes that changed retrieve colors.

Does that make sense? If not happy to discuss.

cozmo · 2018-07-22T18:51:48Z

src/color-retriever/index.ts

+import { Point } from "../Point";
+
+// Stores two RGBA values (0-255, [r, g, b, a])
+export interface QRColors {


My preference would be to return a color as

interface Color { r: number; g: number; b: number; }

As I think it's a better API for the user, and I don't think we get many gains for using typed arrays for this small of data. I also don't think we care about alpha - we ignore it everywhere else.

cozmo · 2018-07-22T18:58:14Z

src/color-retriever/index.ts

+// Color space conversions from http://www.easyrgb.com/en/math.php
+
+// Converts an RGB color ([r, g, b] or [r, g, b, a] - a is ignored) to CIELab ([L*, a*, b*]).
+function rgbToLab(rgb: Uint8ClampedArray): number[] {


My preference would be that these functions took in and returned named params, either in the form of an object, or as named arguments like (function(r: number, g:number, b:number){}` I think this makes them clearer to read and also less prone to error.

cozmo · 2018-07-22T19:01:55Z

src/color-retriever/index.ts

+color space for averaging (with no regard for alpha), and then converted back to RGB. Alpha values are simply averaged
+directly. */
+export function retrieveColors(location: QRLocation,
+ extracted: {matrix: BitMatrix; mappingFunction: (x: number, y: number) => Point; },


Instead of defining this here, might make sense to define an Extracted type in extractor and then reference it here, I feel like that would be a little cleaner per se.

cozmo · 2018-07-22T19:05:10Z

src/index.ts

 }

-function scan(matrix: BitMatrix): QRCode | null {
+function scan(matrix: BitMatrix, sourceData: Uint8ClampedArray, sourceWidth: number, scanOptions: Options): QRCode | null {


Not sure if this makes sense or not, but the width and the height are already available on the binarized bitMatrix. Not sure we need to pass them in again.

david-boles · 2018-07-22T23:01:27Z

Those all sound good, I'll update it soon.

david-boles · 2018-07-23T05:37:13Z

For testing I added some checks for the color space transformations and effectively end-to-end-tests.ts but that only looks at color. For populating the test data I just added on to generate-test-data.ts but that might want to be broken out into a separate script.

cozmo

Left some very basic comments but this looks great! Sorry again for the delay, been awhile since I've been able to sit down and go through PRs/issues. Hope I haven't been blocking you.

cozmo · 2018-08-15T06:25:56Z

src/color-retriever/index.ts

+}
+
+interface CIELabColor {
+ L: number;


Is it expected that the L is capitalized but the other attribute aren't?

Most of the reference material I've seen generally capitalizes them that way... Up to you though, obviously it doesn't follow javascript conventions.

cozmo · 2018-08-15T06:26:44Z

src/color-retriever/index.ts

+
+// Color space conversions from http://www.easyrgb.com/en/math.php
+
+// Converts an RGB color ([r, g, b] or [r, g, b, a] - a is ignored) to CIELab ([L*, a*, b*]).


This comment isn't strictly correct anymore I don't think - Can probably just remove the array descriptions from it?

cozmo · 2018-08-15T06:27:04Z

src/color-retriever/index.ts

+ return {L: l, a, b};
+}
+
+// Converts a CIELab color ([L*, a*, b*] - ignores additional values) to RGB ([r, g, b]).


Same re: this comment no longer needing the array descriptions.

cozmo · 2018-08-15T06:29:35Z

tests/generate-test-data.ts

+ const output = jsQR(imageData.data, imageData.width, imageData.height, {retrieveColors: true});
+
+ await fs.writeFile(path.join("src", "color-retriever", "test-data", t, "output.json"), JSON.stringify(output.colors, null, 2), "utf8");
+ }


I actually don't think we strictly need this, since I think the test cases are sort of...stand alone. Now that we've created them I would expect them to not break as we change the library? If they do we can re-visit needing to autogen them, but for now would be fine leaving them standalone?

Seems useful to leave for adding more color tests but splitting it out makes sense. I'll make it a separate file in the color retrieving folder?

cozmo · 2018-08-15T09:29:34Z

src/color-retriever/test.ts

+function roundComponents(color) {
+ for(var component in color) {
+ color[component] = Math.round(color[component])
+ }


This function doesn't return anything so I don't actually think the tests are doing anything, since they're always comparing undefined to undefined

I think it should work (given the mutability of arrays) but definitely not intentional :)

The mutability doesn't matter here I don't think. This function has no return, and so as such returns undefined. We use it in the assertions above, so they all become expect(undefined).toBe(undefined). Unless I'm missing something?

Oh boy, I must have been reeeealy out of it. NVM

cozmo · 2018-08-15T09:30:46Z

Just left one final comment since I tried to merge this. but realized the conversions tests weren't actually doing anything. Right now there's a merge conflict on master, but feel free to ignore that, I'll handle it since I caused it. That said not really sure how best to fix these tests so would rather defer to you.

david-boles · 2018-08-15T22:18:17Z

No problem, I've just been using a build of my repo for now. I've gotten a bit busier myself but I'll try to sit down and implement your comments today or tomorrow. A replied to a couple of things immediately, I'll leave the capitalization of L up to you.

david-boles · 2019-06-29T18:19:18Z

@cozmo Sorry for disappearing for so long! It's been a busy school year... If I finish up this pull request (I think I had more to do even though you approved it...), still up for merging it?

david-boles and others added 6 commits July 9, 2018 21:29

Implemented color retrieval.

0c921d0

Cleaned up color retrieval implementation.

f0f0f46

Re-implemented using CIELab color space averages (untested) and recom…

9ebb207

…mendations from cozmo

Commenting improvement.

b67dbda

Added testing for color

a151a36

Add option to disable inverting

0be6075

cozmo mentioned this pull request Jul 18, 2018

Add option to disable inverting #78

Merged

david-boles added 7 commits July 18, 2018 20:11

Prepped for merging with cozmo#78

d48b62d

Merge branch 'cozmo-add-option-to-disable-inverting'

19d1eff

Making Travis happy round 1

cd797e6

Making Travis happy round 2

1330a90

Making Travis happy round 3

5506d82

Making Travis happy round 4

9d687e6

Making Travis happy round 5

8eaabcf

Rebuild and regenerated test data.

8323798

david-boles added 3 commits July 19, 2018 12:57

Hacky possible fix to Jest testing serialization issue.

b1e932d

Re-run travis testing

edf523e

Rolled back failed testing fix.

a926880

cozmo reviewed Jul 22, 2018

View reviewed changes

Implemented cozmo's suggestions.

0f46e9e

Rebuilt

a749c3f

cozmo approved these changes Aug 15, 2018

View reviewed changes

cozmo reviewed Aug 15, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to retrieve the colors used in the QR code from the image. #76

Added option to retrieve the colors used in the QR code from the image. #76

david-boles commented Jul 13, 2018 •

edited

Loading

cozmo commented Jul 18, 2018

cozmo commented Jul 19, 2018

david-boles commented Jul 19, 2018

david-boles commented Jul 19, 2018

david-boles commented Jul 19, 2018

cozmo left a comment

cozmo Jul 22, 2018

cozmo Jul 22, 2018

cozmo Jul 22, 2018

cozmo Jul 22, 2018

david-boles commented Jul 22, 2018 •

edited

Loading

david-boles commented Jul 23, 2018

cozmo left a comment

cozmo Aug 15, 2018

david-boles Aug 15, 2018

cozmo Aug 15, 2018

cozmo Aug 15, 2018

cozmo Aug 15, 2018

david-boles Aug 15, 2018

cozmo Aug 15, 2018

david-boles Aug 15, 2018

cozmo Aug 16, 2018 •

edited

Loading

david-boles Aug 16, 2018

cozmo commented Aug 15, 2018

david-boles commented Aug 15, 2018

david-boles commented Jun 29, 2019


		// Color space conversions from http://www.easyrgb.com/en/math.php

		// Converts an RGB color ([r, g, b] or [r, g, b, a] - a is ignored) to CIELab ([L, a, b*]).

Added option to retrieve the colors used in the QR code from the image. #76

Are you sure you want to change the base?

Added option to retrieve the colors used in the QR code from the image. #76

Conversation

david-boles commented Jul 13, 2018 • edited Loading

cozmo commented Jul 18, 2018

cozmo commented Jul 19, 2018

david-boles commented Jul 19, 2018

david-boles commented Jul 19, 2018

david-boles commented Jul 19, 2018

cozmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-boles commented Jul 22, 2018 • edited Loading

david-boles commented Jul 23, 2018

cozmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cozmo Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cozmo commented Aug 15, 2018

david-boles commented Aug 15, 2018

david-boles commented Jun 29, 2019

david-boles commented Jul 13, 2018 •

edited

Loading

david-boles commented Jul 22, 2018 •

edited

Loading

cozmo Aug 16, 2018 •

edited

Loading