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

fix left edge location for first character of the label. #180

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

FoamyGuy
Copy link
Contributor

This fixes an issue that resulted in multiple labels using monospace fonts with different leading characters to not align properly if the shift x values from the bounding box property in the font is different for different characters.

The change hardcodes the left value to start at 0 rather than the shift x value which was actually causing the shift to get "cut out" and no included in the final bitmap, as well as causing alignment to become misaligned with other labels placed at the same x position.

This problem was original reported on the forums here: https://forums.adafruit.com/viewtopic.php?p=954011

There are details and photos in that forum post.

Here are some photos illustrating the difference with this proposed change:

Currently released version:
current_impl

This PR version:
pr_version

If adopted this change would result in the visual x location of labels to change slightly depending on the shift x value for the leading character in whatever font is used. Projects that used larger fonts, or fonts with larger shift x values will see a more substantial change.

This change also has the side effect of putting in the left side padding inside the background box of the label. Previously the left edge of the first character was touching the left edge of the label background rectangle, with this version the shift from the font is respected and used as a left padding.

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good change, definitely good to be consistent. This only effects a single bound box, correct? The "previous" behavior could still be replicated using multiple boxes, right (in case it was desired for some reason)?

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Jan 7, 2023

The change would effect any label whose font doesn't have a value of 0 for the offset of whatever the first character in the label text is.

In the screenshots shown each line is drawn by a separate label, so they each have their own bounding box. I have not done any testing around \ns in the text to see what happens with multi-line labels that have 1 single larger bounding box covering multiple rows of text, it may also need some tweaks, but is outside the scope of this one I think.

It would be possible to replicate the old behavior with the changed library, but it could be a bit tedious if the aim is to actually replicate it 100% pixel perfect. In order to do so, one would need to check the offset value in the BDF font they're using for the first character in the label and then once that value is known they could use it as an offset with either the x property or the anchored_position to "manually" move the label back to where it would have gotten rendered with the currently released version of the library.

If someone where just interested in getting close without necessarily being pixel perfect for all possible different leading characters they could just shift the x position by a little bit and eye ball it until it's lined up how they want.

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all makes sense to me, thanks for explaining!

@FoamyGuy FoamyGuy merged commit 1590d81 into adafruit:main Jan 11, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 11, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SH1106 to 1.2.10 from 1.2.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SH1106#11 from greatest-gatsby/main
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 2.23.0 from 2.22.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#180 from FoamyGuy/fix_left_glpyh_logic

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.1.0 from 7.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#136 from vladak/user_data
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#139 from vladak/tls_port
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#141 from vladak/enable_logger_nits

Updating https://github.com/adafruit/Adafruit_CircuitPython_OneWire to 2.0.4 from 2.0.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_OneWire#28 from tekktrik/dev/fix-annotations
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
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.

2 participants