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

ios: specify EC level/margin for generation #644

Merged

Conversation

markusfisch
Copy link
Contributor

So we can specify a certain error correction level (and margin).

@benjohnde
Copy link
Contributor

Not a big deal but you could add a wrapper function to provide defaults and I would not change the signature as this would break existing code. On the other hand, I am not sure how many people rely on the library and on these functions @alexmanzer.

To simplify creation of barcodes when it is not necessary to
specify EC level or margin.
@markusfisch
Copy link
Contributor Author

markusfisch commented Oct 24, 2023

Good point! I was already playing with the idea of adding a wrapper function, but wanted to keep this initial request as simple as possible. But I think you're right, and so here are two wrapper functions now.

About the function signature - I know this was a bold move - but since we just introduced these functions, there's probably no existing code out there that would break, apart from our own 😉

I moved format up, because width and height are (in theory) optional parameters too (you can give 0 for both to get the smallest possible barcode). So format and contents/data are the only mandatory arguments, and I felt they should be next to each other, just as the are in (our flavour of) the Android wrapper (the default Android wrapper does not support creation of barcodes). So, of all times, this is probably the moment to make this change 😉

@benjohnde
Copy link
Contributor

benjohnde commented Oct 24, 2023

About the function signature - I know this was a bold move - but since we just introduced these functions, there's probably no existing code out there that would break, apart from our own 😉

Haha, I actually liked the change - nothing wrong with the bold move though.

I moved format up, because width and height are (in theory) optional parameters too (you can give 0 for both to get the smallest possible barcode). So format and contents/data are the only mandatory arguments [...]

True, I can relate. Just wanted to mention that overall. LGTM, the release notes should probably contain a gentle hint if @axxel agrees to the overall change.

@axxel
Copy link
Collaborator

axxel commented Oct 25, 2023

In general, I'm totally fine with those changes as things stand right now. That said I'd like to refer to the age old writer API discussion (again, I know), especially this and following. I wished I had made more progress there by now but I have not. Bottom line: I'd like to introduce an EncodeHints struct along the lines of

struct EncodeHints {
  BarcodeFormat format;
  int/float scale; // AND/OR widthHint, heightHint
  int ecLevel; // the current interpretation of the numbers 0-8 is a mess and will not last
  int margin; // currently meaning pixels but should rather mean modules (imho)
  ... // to be expanded
}

See here and here for details about the scale/width thingy.

This whole size issue might convince me to separate the process into two steps (similar to what libzint is doing):

  1. Generate an abstract "symbol" result that knows how many modules it contains, i.e. it's minimum size in pixels.
  2. Supply member functions or free functions that allow to render this symbol into a bitmap of a certain size or maybe an SVG.

The SVG route would not have this size issue at all and I'm wondering whether that would not be the best result type to begin with? iOS should have some splendid options for rendering some SVG content in arbitrary sizes on the screen, right?

Maybe start with writeToSvgFromString or writeSVGString or however this aught to look to make the Swift auto overloading feature work? Even if we end up with the mentioned two-step process, those names would not collide and could stay around for backward compatibility.

For encoding/writing barcodes.
@markusfisch
Copy link
Contributor Author

Well, while I can see why you want to get rid of width and height in favor of a simple scale parameter, as well as cleaning up ecLevel and margin, and even that it would be better to just return a clean 1x1 BitMatrix, I think all of this is probably a bit beyond this small extension of the iOS wrapper 😬 To change these things would affect all wrappers, of course, and we would like to have a way to specifiy these things now.

So I would propose to tackle these changes in a second, bigger step. Of course, that would mean breaking changes, but that's what it means for all other wrappers anyway.

I'd like to introduce an EncodeHints struct…

Something like this?
If you like this, I would merge it in this branch.

iOS should have some splendid options for rendering some SVG content in arbitrary sizes on the screen, right?

Unfortunately not 🙈 iOS doesn't support SVG directly. You can only bundle it in the assets of an app, but there are no native functions to work with SVG on iOS yet.

This whole size issue might convince me to separate the process into two steps

This is very similar to what I already do in my flavor of the Android wrapper, as you know. So I totally agree with that. But I think this would be another step for the iOS wrapper.

@axxel
Copy link
Collaborator

axxel commented Oct 26, 2023

Of course, that would mean breaking changes

My idea to bring this up now was to try to prevent (or limit) breaking changes in the iOS wrapper considering that this functionality is not really used by anyone yet (except you, it seems).

If there is no SVG support in iOS, how do you currently render the result? Do you have an area where you want to render the code into and then request just that size in pixels? Is there any other vector graphic API / data format that one could use or would it be required to "manually" draw primitives directly in some form of rendering API?

Thanks for the suggestion with the ZXIEncodeHints. Please merge that as you suggested. Looks like a good starting point.

My two-step idea has similarities to your Android code but as far as I saw, the difference would be that the first stop would not have anything like a scale or width only the rendering part.

One thing that I'd definitively like to add/change before a release is that the width/height parameters are only going to be a hint and the returned image would then contain just the symbol with requested quiet zones (note the current implementation already interprets margin as exactly that) but no extra white background just to produce a specific size. I had a short look at the Inflate helper and will later hack something with inverted sign of width to switch between the current and the new behavior, so MultiFormatWriter can be used for both until we have a generally new WriteBarcode style API on the c++ level.

@markusfisch
Copy link
Contributor Author

My idea to bring this up now was to try to prevent (or limit) breaking changes in the iOS wrapper considering that this functionality is not really used by anyone yet (except you, it seems).

Wouldn't that rather be an argument to merge this as it is when we are the only ones that would have to deal with the breaking change? I mean we would happily deal with it later and use it in this form in the meantime 😉

If there is no SVG support in iOS, how do you currently render the result?

ZXIBarcodeWriter.write*() already returns an CGImage, which is basically a bitmap. So currently, the BitMatrix is inflated here in the iOS wrapper.

At the moment, we don't use any vector formats on iOS for barcodes. But this will change at some point, of course. I don't really know what we will use for that - that's a question @alexmanzer can maybe answer.

Thanks for the suggestion with the ZXIEncodeHints. Please merge that as you suggested. Looks like a good starting point.

Merged.

My two-step idea has similarities to your Android code but as far as I saw, the difference would be that the first stop would not have anything like a scale or width only the rendering part.

Yes, that is only there because it is required for now. I would very much like to remove width/scale for the first step, too!

I had a short look at the Inflate helper and will later hack something with inverted sign of width to switch between the current and the new behavior, so MultiFormatWriter can be used for both until we have a generally new WriteBarcode style API on the c++ level.

Sounds good! 👍 Although using negative values definitely feels like a hack 😉 But would be okay!

@alexmanzer
Copy link
Contributor

At the moment, we don't use any vector formats on iOS for barcodes. But this will change at some point, of course. I don't really know what we will use for that - that's a question @alexmanzer can maybe answer.

Current state:

  • iOS still can't handle vector images like SVGs in (UIKit's) UIImage or SwiftUI Images. There are a lot of 3rd party libs (like SVGKit) for this, but I'm not the biggest fan of even more external dependencies.
  • UIImage, CIImage, CGImage are all pixel based Classes/Implementations
  • There are workarounds without external dependencies like "use a webview, because they can render SVG's", but this is also not a real solution IMO
  • The same is valid for PDF vector images
  • Yes, I know, since Xcode12 you can use SVG images in an app bundle. But that is not the same as downloading/creating a SVG and show it in an ImageView. This is still not possible.

For iOS development, SVG is still a "foreign body" and you need external libs to use it. It's sad - but that's the way it is.

Please correct me, if I'm wrong @benjohnde, @parallaxe and all the other iOS Devs out there.

@axxel
Copy link
Collaborator

axxel commented Oct 26, 2023

Thanks @alexmanzer to chime in from your vacation ;). I'll happily trust your expertise regarding SVG and iOS. But then let me repeat my earlier question: If there is no SVG support in iOS, how do you currently render the result? Do you have an area where you want to render the symbol into and then request just that size in pixels?

@alexmanzer
Copy link
Contributor

Yes, at least "something like that".

Example:
You have an UIImageView that can display a pixel image.
Since there are devices with different pixel densities (iPhone 11 has x2 and 11 Pro has x3), you would theoretically need multiple images with different resolutions for a pixel-perfect display.
So if a UIImageView in the app is 200x200pt (pt = points), then you would need a 400x400px image for 2x and a 600x600px image for 3x.

Long Story Short:
In practice, from my experience, this is how you do it (if pixel-perfect rendering is not crucial to life):
You just use an image with high enough resolution for the best possible display. And since you have enough computing power anyway, you don't care that iOS has to downscale the image for 2x devices. For a QR code with e.g. 64x64 modules it doesn't really matter in real life, because a single module is rendered with e.g. 9x9px.

(With the scaling mode you should be careful not to use a mode that interpolates bilinearly, but only scales up-down according to e.g. nearest neighbor. But even this is not "decisive for the war" in most real world applications.)

@alexmanzer
Copy link
Contributor

alexmanzer commented Oct 26, 2023

So from a pure iOS point of view I would say that it is either fine:

  • to specify the needed image size
  • or you just get a bitmap with 1px = 1 Module, which you scale up yourself.

By the way, in my opinion this is the most effective way to handle e.g. a QR code as an image anyway. A b/w PNG with 64x64px (for 64x64 modules) is most likely much smaller than a SVG, where this structure is "described".
But that's another discussion, I believe. 😉

For this PR, I think it would be fine to just include the pixels needed in height and width.

@axxel
Copy link
Collaborator

axxel commented Oct 26, 2023

Thanks for the backgound info. I just wanted to make sure the user of the API is aware that just because they specified a width of 200 does not mean they'll get it. The resulting size might be lower or even higher if the symbol size is in modules is larger than 200.

As for the SVG vs Bitmap discussion: once we switch to libzint, we'll have the option to ask for text rendered into the result (think EAN13). And then SVG makes a lot of sens. In-fact rendering the symbol with a module size of 1px will produce a barely readable text.

@axxel axxel merged commit f50cd5c into zxing-cpp:master Oct 26, 2023
10 checks passed
@markusfisch
Copy link
Contributor Author

Thanks for merging! 👍

As for the SVG vs Bitmap discussion: once we switch to libzint, we'll have the option to ask for text rendered into the result (think EAN13). And then SVG makes a lot of sens. In-fact rendering the symbol with a module size of 1px will produce a barely readable text.

Ah, now I understand why you're focussed on SVG! Makes sense!

@markusfisch markusfisch deleted the ios_generate_with_ec_level_and_margin branch October 30, 2023 13:31
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.

None yet

4 participants