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

write_left,right rounds to pixel. temperature_color robus to nil temp #3

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

ogrefish
Copy link
Contributor

this helps cairo drawn text in the drive temperatures and gpu top list be sharp
also avoids crashing when temperature is nil, which is nice while configuring hardware

@philer
Copy link
Owner

philer commented Apr 22, 2020

Hey, thanks for the PR! If that does away with the blurry text I'll be very happy.
I'll test it tonight but I'll leave a few comments for now. I'm rather picky on formatting, please bear with me ;D

src/cairo_helpers.lua Outdated Show resolved Hide resolved
x, y = cairo_user_to_device(cr, x, y)
-- https://scriptinghelpers.org/questions/4850/how-do-i-round-numbers-in-lua-answered
x = x + 0.5 - (x + 0.5) % 1
y = y + 0.5 - (y + 0.5) % 1
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure about this one? Following the link you've provided I can only find the standard math.floor(x + 0.5) approach. If this is some lua hack I don't know yet please put it in a util.round helper and add an explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not a lua thing, it's just another way to round to an integer. It adds 0.5 and then subtracts the remainder that doing so pushes the sum over an integer.

BlueTaslem's answer in that link says this method is about 20% faster than floor(x+0.5). I'm fine to change it if it's hard to maintain/read. Can also refactor the rounding routine into its own function.

Copy link
Owner

Choose a reason for hiding this comment

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

Honestly I'd just prefer the inline floor(x+0.5). It's simpler and this is definitely not the bottle neck.

src/cairo_helpers.lua Outdated Show resolved Hide resolved
src/widget.lua Outdated Show resolved Hide resolved
@philer
Copy link
Owner

philer commented Apr 22, 2020

I've taken a closer look now and unfortunately it doesn't look like this changes text sharpness.

The referenced FAQ section is talking about straight lines aligned to pixels, like the ones used to draw the various boxes. If anything it'd make sense to use this in widgets like Bar and Graph, but these should already be aligned pixel perfect. Text isn't straight lines and so will get anti-aliased either way. The changes here only align the top left corner of the text's bounding box, which doesn't do much.

In addition, transforming back and forth between user and device space is only relevant if there is a transformation matrix. The only transformations used in the layout system are translations by full integer values, which do not change anything about the rendering sharpness.

The fact that the text test passes on the PR confirms that there is no difference in rendering. Some impromptu logging also shows that the values don't change before and after rounding. If you're getting different results on this, please let me know!

As such I won't merge the changes involving coordinate rounding unless there is some benefit I've missed.

If you want to remove them, though, I'd be happy to pick up the changes you did to w.temperature_color.

edit: I accidently sent this before I was done writing, sorry. :/

@ogrefish
Copy link
Contributor Author

I've taken a closer look now and unfortunately it doesn't look like this changes text sharpness at all.
The referenced FAQ section is talking about straight lines aligned to pixels, like the ones used to draw the various boxes in

It made text sharp on my machine. I'm running Arch with conky-cairo, though, and the test-text has always failed for me. Maybe there is some difference?

@philer
Copy link
Owner

philer commented Apr 22, 2020

Could you send me a screenshot before/after?
I've already got another open issue (#1) from someone who's text is rendered differently. I may be wrong about me conclusion above but if so I want to figure out what's going on first.

@ogrefish
Copy link
Contributor Author

Sure.
Before: https://pasteboard.co/J51fvqR.png
After (with rounding): https://pasteboard.co/J51fJPc.png
Note the drive temps.

Conky:

conky 1.11.3_pre compiled Mon 20 Apr 2020 09:58:05 PM PDT for Linux 5.6.4-arch1-1-fsync x86_64

Compiled in features:

System config file: /etc/conky/conky.conf
Package library path: /usr/lib/conky


 General:
  * math
  * hddtemp
  * portmon
  * IPv6
  * Curl
  * iconv
  * wireless
  * support for IBM/Lenovo notebooks
  * nvidia
  * builtin default configuration
  * old configuration syntax
  * Imlib2
  * OSS mixer support
  * apcupsd
  * iostats
  * ncurses
  * Internationalization support

 Lua bindings:
  * Cairo
  * Imlib2
  * RSVG
 X11:
  * Xdamage extension
  * Xinerama extension (virtual display)
  * Xshape extension (click through)
  * XDBE (double buffer extension)
  * Xft
  * ARGB visual
  * Own window

Cairo:

local/cairo 1.17.2+17+g52a7c79fd-2
local/cairomm 1.12.2-3
local/conky-cairo 1.11.3-1
local/lib32-cairo 1.17.2+17+g52a7c79fd-2
    The pixel-manipulation library for X and cairo
local/python-cairo 1.19.1-1
    Python bindings for the cairo graphics library

@philer
Copy link
Owner

philer commented Apr 22, 2020

Okay that definitely looks like an improvement. Apparently the vertical translation has some effect after all, even though I cannot reproduce it. Even shifting content by 0.5 pixels does not give me the same result.
Maybe it depends on the font file or it's because we have different versions of cairo…

@philer philer merged commit 5498d21 into philer:master Apr 22, 2020
@philer
Copy link
Owner

philer commented Apr 22, 2020

Thanks!

@ogrefish
Copy link
Contributor Author

Yes I noticed that shifting horizontally by 0.5 pixels did not work for me either. I didn't investigate, tho.
It does seem related to the open issue.

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.

3 participants