-
Notifications
You must be signed in to change notification settings - Fork 27
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
Auto bitmap sizing from Metadata #34
base: master
Are you sure you want to change the base?
Conversation
Love the concept -- will definitely be an improvement in user experience. Have a couple of high level comments:
I have a handful of comments on stuff like formatting, style and performance. Happy to leave these in a review if you want to make changes yourself, or I can make the changes directly -- let me know what you prefer. |
And when it comes to comments, feel free. It's your repo, I'm just someone who's a little too happy to help :p And styling is a personal point of contention because I usually format with 4 spaces (I can work with 2, don't worry). But I use VSCode's C++ formatter (I think) and pushing a commit with that formatting would cause too many changed lines for no functional change. |
Super happy to have you helping out. It's been great. Only asking your preference here because I want to make it as comfortable of a contribution environment as I can. :)
The UI always populates it, but the API doesn't enforce that it exists (because at the time, it was only useful for the UI). The bin files are straight-up bitmaps (the data structure, not the file format). The number of pixels in the image is
Haha - I only care about formatting for consistency. If it helps, I use |
Glad to hear it! It's a fun project that I see many uses for. Especially since it just becomes much easier to make projects that can use an e-paper display and is okay with just reading from MQTT. Much more efficient than getting a Kindle, jailbreaking, getting a web browser page set up to not go to sleep, get a proper JS interface to read, scream at JQuery not working on old kindles, cry w̵̨̨̻͙͎̰̹̱̖̲̥̦̫͇͊̈̌̐̃̑͒̀̀̈́̚͜h̴̡̜͈̅̃̇̎̉̆̀͗͜ÿ̵̦͑̽̓̃̓͌̍͠ ̷̛͔̗͙̥̲̫̠̟̭͑͐̇̾͊̅̔̀̽̿̊͑͂͠ͅa̸̢͍̘̤̻̟̝̜͖̰͎͍͆̔̈́̉́̃̒̐͝ṁ̵͔̘͉̩̹͓͎̲̞̬̟̳͖͑ą̶̹̲͕̜̥͊̌̾̆̀̆̃̊͆͗͂̿̀̀͘z̵̨̤̗̼̟͇̙̎̄ͅò̶̼̱̙̟͎̬͔̲̳̠̲̉́̉̆̐̽͆̀͌́̇̚͠n̴̨̨̗̲̙̣̫̯̜͌̀͒̉ͅ ̸͇͇̳̺̎̌ẃ̵͕̝̞̙͖͈̪̬͚̪̆̎͒̋̆̂̚͝h̸̢͓̯̦̾͜ÿ̶̧̨̡̦͉͈̪͍̮̗̤͍͔̙͎̎̾̈́̎̾̓͋̐̕͝. So I'm happy to help!
While I don't doubt you and I haven't gone too much in-depth into it, I'm just wondering about more rectangular images! Like a 1:2 aspect ratio.
oh shiz you're right, I didn't see that. Hopefully I can wrangle VSCode to take it, then. Linux-VSCode implementation of certain linters/formatters refuse to be discovered. Doubly so with PlatformIO in the mix. |
Serial.println(F("WARN - tried to get dimensions from bitmap that doesn't exist: ")); | ||
Serial.println(bitmapName); | ||
Serial.println(metadata_filename); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calculate default based on bitmap file size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we strike this or keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see three options:
- Calculate the default somewhere (was thinking this was the appropriate place)
- Calculate a default on the API side and generate a default metadata file so that we can treat the existence of a metadata file as an invariant (in which case what you have here is fine)
- Refuse to save bitmaps that don't have metadata files supplied (same deal -- we get the invariant)
I was originally proposing (1), but (2) hadn't occurred to me until now, and I think I like that better. Open to whatever makes most sense to you, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~~2, I feel is best. I think it should save the bitmap even if there isn't a supplied W and H with a default set by us ~~
Actually, no. 3. Since if you use the resizer in the bitmap editor, it will screw up the already supplied bitmap.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UI always submits the dimensions of the image every time it's saved, so that shouldn't be a problem.
Either (2) or (3) seem fine.
Co-Authored-By: Chris Mullins <[email protected]>
Just left the fiddly suggestions -- I can make those changes myself if you prefer. re: non-square images, you're right, there's no way to do this without metadata. Just meaning to suggest a default behavior that makes sense as often as possible. Option (B) is the only alternative I see. That's fine, just makes the API more rigid. |
oh, and on the UI front -- it's possible that parts of the preview canvas is relying on the width/height in the definitions. If you're getting strange behavior there (errors or distorted images), that's probably why. Happy to help out with that, just lmk. |
Co-Authored-By: Chris Mullins <[email protected]>
A more rigid API isn't always bad. It just depends. And having it at least generated automatically when you supply the file, W, and H, it shouldn't be an issue (in my eyes. I could be wrong).
Yeah, I was having that issue from minute one. It was fixed by refreshing/reopening the template editor. Having it re-render it upon the bitmap change should be easy enough? |
lol, of course. Just meant that in the way of expressing a cost. Uploading a bitmap with curl, for example, would be something like this:
It works, it's just awkward.
Yeah, shouldn't be a problem. There's a fair amount of plumbing involved to make this happen, but should be mostly straightforward. Let me know if you want a hand :) |
NoteYou will want to remake your examples after this, likely. Also, how would you do the upload_bitmaps script when it comes to their sizes? unless they are all one size. What if we scaled it on the ESP before displaying?Size won't matter as much, you can use the same picture multiple times without wasting SPIFFs space, and the issue with the upload_bitmaps will be lessened (but it will still be there) |
Great points. If I were writing the script for myself, I'd probably use identify -format '{"width":%w,"height":%h}' "${file}" \
| curl -s "${server}/bitmaps" -F filename=@${file} -F "metadata.json=@-" That script could use some love, though. Pretty raw as-is. (also not clear to me it's terribly useful. if batch uploads are worthwhile, should add that functionality to the UI).
This is a cool idea, but scaling images properly is surprisingly challenging. I even avoid it in the UI by punting it to the browser (by either up- or down-scaling the image in an I could definitely be convinced otherwise, but I'd prefer to keep bitmaps fixed size. Both to avoid the additional complexity, and because doing it well is gonna be pretty hard. |
Okay so I had a similar issue with scaling a while ago when I was creating an ESP32-connected Thermal Printer (receipt printer). The QR codes I would generate would be too small, and I also wanted it to be padded to the center of the receipt. I have to run somewhere quickly, but I want to send this to you before I go so you can keep it in mind and maybe think about using it. CPP #include "Arduino.h"
#include "qrcode.h"
#include "QRCodeStream.h"
// Written by Arix Zajicek
int QRCodeStream::available()
{
// For simplicity, pretend the bytes come one at a time.
if (done) return 0;
else return 1;
}
int QRCodeStream::peek()
{
}
void QRCodeStream::begin()
{
// Set the starting index
byteIndex = 0;
scaledQrSize = scale * qrcode.size;
lPadding = (384 - scaledQrSize) / 2;
bmpWidth = scaledQrSize + lPadding;
if (bmpWidth % 8 != 0){
bmpWidth += 8 - (bmpWidth % 8);
}
}
int QRCodeStream::debug(int which){
return 1;
}
int QRCodeStream::read()
{
if (byteIndex > bmpWidth / 8 * scale * qrcode.size) return -1;
int data = 0;
int row = byteIndex / (bmpWidth / 8);
for (int col = (byteIndex * 8) % bmpWidth; col < (byteIndex * 8) % bmpWidth + 8; col++) {
bool pixel = false;
if (col >= lPadding & col <= lPadding + scale * qrcode.size) {
pixel = qrcode_getModule(&qrcode, (col - lPadding) / scale, row / scale);
}
data = data | (pixel << (7 - (col % 8)));
}
byteIndex++;
return data;
} Header #include "Arduino.h"
#include "qrcode.h"
// Stream a scaled QRCode one pixel at a time.
class QRCodeStream : public Stream
{
public:
QRCodeStream(QRCode &code, uint8_t scale = 1)
: qrcode(code), scale(scale) {}
// Output: writing to this stream is a no-op.
size_t write(uint8_t) {}
void flush() {}
// Input delivers the QRCode pixels.
int available();
int peek();
void begin();
int debug(int which);
int read();
int bmpWidth;
int scaledQrSize;
private:
QRCode &qrcode;
uint8_t scale;
int lPadding;
int byteIndex;
bool done = false;
}; This is written by my Cpp Wizard friend, not me. |
Yeah, naive up-scaling is pretty straightforward (it's called nearest-neighbor interpolation): each pixel in the new image is mapped to a single pixel in the source, and its value is determined solely from it. That's all that's going on in that code sample. But:
It's not that I don't think I can do it -- I definitely can. It even sounds fun. I just don't want to venture too far outside of the core value of the project :) |
(I could be totally wrong, however.) How does the scaler in the bitmap editor work?
|
I dodge the problem in the UI by making the browser do the work (browsers obviously have to be able to scale images, and they do it really well). |
I look at that and think "Well just make the lines thinner, dummy," but that's not a reasonable solution.. This is a topic for another time, perhaps. Do you feel it's still worthwhile to add this automatic size-detection or no? |
Yeah, it's a hard problem to solve well generally. Agreed -- let's discuss elsewhere if we want to continue. Automatic size detection definitely worth it. This is a good change. Think we've got (A) API hardening, and (B) making sure the UI works with the change left. I'm happy to help out with either or both of those, but will leave it to you unless you let me know what you'd like help with :) |
I might take a crack at the API, but the UI is something totally esoteric to me. If you could lend a hand there, that would be much appreciated. I haven't delved into the API section of the project yet, but it shouldn't be that bad, right? |
Sure, sounds good. I'm playing around with a wacky idea right now (adding a Lua scripting engine), so might be a bit before I get to it. Nah, I expect the API stuff to be mostly straightforward. Let me know if you'd like some pointers. :) |
Lua, huh? Haven't heard that since The Bodge and Garry's Mod. |
This does work so far, but needs the following things if you feel this is worthy.
For issue #33