-
Notifications
You must be signed in to change notification settings - Fork 8
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
Improve sample pattern #55
Conversation
So this is what it looks like when it is handled by capable hands! :) I think that counting dots beginning with 1 would make counting easier, and the values more rounded. :) Other than that, perfect. |
@tomek-szczesny, good point! My new favorite Turing test is, "count to 10". 😂 |
Fine with me 😅 |
38078c4
to
06b164a
Compare
def _compute_total_width(bitmaps: Sequence[Image.Image], padding: int) -> int: | ||
number_of_bitmaps = len(bitmaps) | ||
width_of_bitmaps = sum(b.width for b in bitmaps) | ||
|
||
# We alternate bitmaps and padding, and have bitmaps on each end. | ||
number_of_paddings = number_of_bitmaps - 1 | ||
width_of_paddings = number_of_paddings * padding | ||
|
||
total_width = width_of_bitmaps + width_of_paddings | ||
return total_width |
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 am not sure this decomposition of the equation to calculate total width is neccessary. It is too detailed.
You could just replace self.PADDING
with padding
in the original code.
I am fond of the idea to create intermediate total_width
variable, since it helps readability, but to extract a one-liner to a function seems to me as an exaggaration.
Also, if you do extrac to a separate function, why not make is a part of the class?
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 made it verbose because I don't ever want to make an effort to reconstruct the reason why I did - 1
. Also, splitting it off as a function makes it easy to ignore the uninteresting detail, even if it could be done as a one-liner.
I don't see what the width computation has to do with the RenderEngine
interface, which is why I didn't add it to the class.
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.
By looking at the sample pattern, I still not sure I truly understand how to use it, i.e. figure out how many pixels are shown. Maybe a small markdown document will help...
Thanks for this contribution!
I think we should eventually support multiple sample patterns (ijn the CLI we can pass the pattern number)
I need to make another set of photos for my guide after adding the |
Partial draft of a markdown doc |
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.
Good start :) Added some tiny corrections and suggestions.
doc/margin-calibration-howto.md
Outdated
@@ -0,0 +1,44 @@ | |||
# How to calibrate the label margins of DYMO printers | |||
|
|||
Each model of DYMO printer has a stationary print head of a given width. |
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.
FYI: In the source it is referred to as height. (label height, etc)
doc/margin-calibration-howto.md
Outdated
and have a 64 pixel print head with a 9mm printable area. | ||
|
||
The LabelManager PC II also supports 19mm and 24mm D1 tapes | ||
and has a 128 pixel print head with a 19mm printable area. |
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.
18 mm (instead of 19mm)
|
||
The LabelManager PC II also supports 19mm and 24mm D1 tapes | ||
and has a 128 pixel print head with a 19mm printable area. | ||
|
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 don't know if you want to list them all, but this page lists the tape options of several printers:
https://www.labelcity.com/dymo-d1-label-tape-compatibility-guide
Just mentioning that the PC II is not the only one to support 24mm
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.
Maybe add a little hands-on section with the commandline argument and what the user needs to do?
Like:
- Print label with labelle --test-pattern=64
- Look at missing pixels, note down first and last
- Print label with labelle --test-pattern=128
- bla bla bla
- Submit issue with folllowing pictures/data
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.
Actually all we need is for someone to print a test pattern once, with a sufficient number of dots.
We could ask for 256 dots and then to submitting the issue here, keep it simple.
In fact, 256 could be the default value of the --test-pattern switch.
@FaBjE, thank you so much for your review, your comments are extremely helpful! I hope to get to it soon. (I'll also try and get to #51 soon.) In order to be able to describe the general calibration procedure, would you be able to provide a few examples using this new test pattern with your 9mm tape? I'm thinking perhaps one test pattern with 128 px, and then another with 93 px? (My idea is that the top of your tape should be at pixel #91, so with 93 px there should be two rows cut off from the pattern on the top.) |
No worries. I have ordered 6mm and 19mm tape, they are in transit to me. I expect them in about a week. Practical issue though, the branch you are working on doesn't support my printer (yet 😉) |
a692667
to
7fea294
Compare
I just made another very quick pass on this. Still todo:
I just produced a printout with some defects which I believe are due to dust on the print head, so I'll take the opportunity to explain cleaning. |
This is now much better thanks to your feedback @FaBjE! I have some structural changes in the works in order to bring |
doc/margin-calibration-howto.md
Outdated
|
||
The goal of calibration is to determine: | ||
|
||
1. How many pixels does the print head have? |
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.
3rd goal: Determine active area size of the printhead (mm)
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.
What do you mean exactly? Do you mean that we should measure the height of the pattern in mm so that we can compute px_per_mm
?
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.
Exactly. To make sense, this needs to be done on the largest supported tape/label size.
I'm pretty happy with this now. I'm happy to receive any further feedback that anyone may have. Follow-up work for future PRs:
|
Merging and releasing this so that we can use it for #62 |
--sample-pattern 64
on 9mm tape on LabelManager PnP:--sample-pattern 128
on 9mm tape on LabelManager PnP:(This makes it easy (for me) to count backwards from the 80 and see that there is a faint row 65.)
To accommodate this, I rendered the test pattern as a
HorizontallyCombinedRenderEngine
. In order to fit the bitmaps into theHorizontallyCombinedRenderEngine
, I had to wrap theImage
class as a_ImageRenderEngine
since thePictureRenderEngine
takes aPath
object instead of anImage
.CC also @FaBjE for review