Refactoring RXingResultPoint usage

[Asha20]

Since this is a library that works with images, naturally there's a ton of 2D point math going on in the code. I feel like putting in effort here will have a big return on investment, since it's such an integral part of the program logic.

Some points that can be improved in particular:

1. The struct seems to be mostly passed around by reference. This means we end up with dereferences everywhere and the code is harder to follow. The struct itself only holds two floats, so passing it by value would be better I think.
2. Point logic can be refactored in some places to make the intent clearer.
3. Some functions expect rectangles, which are typed as &[RXingResultPoint]. A dedicated rectangle type would be better here for type safety.

I'm opening a PR that addresses the first point, and I'd like to tackle points 2 and 3, though that's a more long-term goal.

Goals

• [ ] Rename RXingResultPoint to just Point
  • [x] Rename in this repo
  • [ ] Remove references to RXingResultPoint and similarly named methods in the rxing-one-d-proc-derive crate
• [x] Remove the MathUtils module, since it holds duplicated logic
• [x] Remove the ResultPoint trait (all uses in repo removed, but the type still remains because of proc macro)
• [x] Remove the Point type in the aztec module
• [ ] Replace all f32 parameter pairs and (f32, f32) tuples with Point
• [ ] Refactor point math code (use a + b instead of let x = a.x + b.x; let y = a.y + b.y)

[hschimke]

I'll add to this by saying: we might do the same math in multiple places. I know that there is a module where result point math is done, a place in math_utils, and a bunch of op implementations as well. It all over the place. I think in the c++ ported parts of datamatrix I'm more likely to have passed it by value.

[Asha20]

I see that besides RXingResultPoint there's also a Point struct and a ResultPoint trait. What do you think about removing the latter two and renaming RXingResultPoint into just Point or something similar? All it does is it holds X and Y coordinates, so the RXingResult- suffix feels unnecessary since the struct doesn't hold anything RXing-related.

[hschimke]

I feel fine with that chain of logic. The Point is probably from something I ported from C++ and was in an hurry and didn’t spend any time figuring out what I had already done. The ResultPoint is because the java has an interface for it, but I don’t know how useful that is. I think there might be something to be said for making a generic version of it that can handle f32, f64, u32, u64. The C++ version does that, but I don’t remember why.

[Asha20]

I've edited the original comment to include a list of goals for this issue. Let me know if you have any suggestions or comments.

[hschimke]

I’ll have to double check, but I don’t think this should require a full version bump to 0.4.0.

Well, now that I’ve typed that I think I am wrong. We do expose that through both the serde binding and through the ResultMetadata return value. I don’t mind moving to 0.4, but I’d like to batch up some other breaking changes and do them together.

Other things I’d like to do in 0.4: possibly migrate to using a HashSet for the EncodeHints and DecodeHints because the HashMap has never been rust reasonable and only made sense in Java, POSSIBLY actually do the work to move the reader chain that includes Binarizer, LumaSource, and Reader to use generics instead of &dyn Trait. I should make a milestone for that.

[Asha20]

If renaming RXingResultPoint is a breaking change, we can still refactor the repo to use Point and use an alias to not break things for the user:

pub type RXingResultPoint = Point;

Once we're reading for a version bump, it'll be as simple as removing the alias. Does this sound good or am I missing something else besides the renaming that also makes it a breaking change?

I've already created an alias type actually; I had to because a proc macro needed it; take a look at this commit.

[hschimke]

That should be fine, actually (there is another rust trick I forgot!).

In the not so distant future I’m going to move those macros into the main repository and rearrange things so that the root of this repository is a workspace with the proc-macro crate and the main library (as suggested in #7). That seems less prone to failures when making changes across the two.

[hschimke]

Reopened because it was autoclosed when I merged in #13

[Asha20]

Yeah, that's a bit sucky. From what I can tell, the only way to link an issue inside a PR without write privileges is by writing Closes #number, but it auto-closes the issue as well. Wish there was a Related to #number that linked but didn't auto-close.

Anyway, next up I'm planning to tackle the various number conversions related to point creation, as well as replacing various (f32, f32) pairs with actual point values.

[hschimke]

fe8f89cd29960f70dce641eb08f3f01d0d71c462 addresses "Remove the Point type in the aztec module"