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

Qt: Add antialiasing to traffic graph widget #16153

Merged
merged 1 commit into from Jul 2, 2019

Conversation

@JosuGZ
Copy link
Contributor

commented Jun 5, 2019

Before:

image

After:

image

@fanquake fanquake added the GUI label Jun 5, 2019
Copy link
Member

left a comment

Concept ACK. Can you check if rounded join style looks better? See https://doc.qt.io/qt-5/qpen.html#join-style.

@@ -65,6 +65,7 @@ void TrafficGraphWidget::paintPath(QPainterPath &path, QQueue<float> &samples)
void TrafficGraphWidget::paintEvent(QPaintEvent *)
{
QPainter painter(this);
painter.setRenderHints(painter.renderHints() | QPainter::Antialiasing);

This comment has been minimized.

Copy link
@promag

promag Jun 5, 2019

Member

Not sure if relevant but could be after fill?

This comment has been minimized.

Copy link
@JosuGZ

JosuGZ Jun 5, 2019

Author Contributor

I believe configuration should come before usage.

This comment has been minimized.

Copy link
@promag

promag Jun 6, 2019

Member

You can change render hints at any time.

Anyway, you can simplify this:

painter.setRenderHint(QPainter::Antialiasing);

This comment has been minimized.

Copy link
@JosuGZ

JosuGZ Jun 6, 2019

Author Contributor

You can, but should you?

Your suggestion is good, pushing.

This comment has been minimized.

Copy link
@promag

promag Jun 6, 2019

Member

You can, but should you?

It depends if fillRect is faster without antialiasing - in this case it might make no difference as the rect is aligned and fills the entire surface.

More generally when you "paint/compose" an image you might want some parts/primitives antialiased and others not.

@JosuGZ

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Concept ACK. Can you check if rounded join style looks better? See https://doc.qt.io/qt-5/qpen.html#join-style.

That is a different part of the code. I can't really see the difference, probably because the points are too close. It looks like this:

image

I can push the change anyway.

diff --git a/src/qt/trafficgraphwidget.cpp b/src/qt/trafficgraphwidget.cpp
index 97a8599fb..4c245ad0c 100644
--- a/src/qt/trafficgraphwidget.cpp
+++ b/src/qt/trafficgraphwidget.cpp
@@ -109,14 +109,18 @@ void TrafficGraphWidget::paintEvent(QPaintEvent *)
         QPainterPath p;
         paintPath(p, vSamplesIn);
         painter.fillPath(p, QColor(0, 255, 0, 128));
-        painter.setPen(Qt::green);
+        QPen pen(Qt::green);
+        pen.setJoinStyle(Qt::RoundJoin);
+        painter.setPen(pen);
         painter.drawPath(p);
     }
     if(!vSamplesOut.empty()) {
         QPainterPath p;
         paintPath(p, vSamplesOut);
         painter.fillPath(p, QColor(255, 0, 0, 128));
-        painter.setPen(Qt::red);
+        QPen pen(Qt::red);
+        pen.setJoinStyle(Qt::RoundJoin);
+        painter.setPen(pen);
         painter.drawPath(p);
     }
 }

If I have time I might play a bit with the whole graph to make it better though. That was initially my idea but this fix was simple and looks much better :)

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

ACK
Code change and result looks good to me, haven't tested, we should probably try it out on a few OSes to be sure the setting doesn't cause problems.

@promag

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

That is a different part of the code. I can't really see the difference, probably because the points are too close. It looks like this:

Thanks for testing, I've also checked it.

I think this is a nice improvement and it doesn't take more CPU to be that noticeable (measured in macos with instruments with and without this change).

Other visual improvements are possible but this is fine as it is. ACK a89c808.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Tested on OSX with HiDPI (retina) screen and I can't see any difference. I believe that using HiDPI on OSX sets antialiasing by default (unsure though).

@JosuGZ JosuGZ force-pushed the JosuGZ:antialiasing branch from a89c808 to 4211bdb Jun 6, 2019
@promag

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

ACK 4211bdb.

@hebasto

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

https://doc.qt.io/qt-5/qpainter.html#rendering-quality:

The QPainter class also provides a means of controlling the rendering quality through its RenderHint enum and the support for floating point precision: All the functions for drawing primitives has a floating point version. These are often used in combination with the QPainter::Antialiasing render hint.

Could using of floating point versions for drawing primitives (e.g., QLineF) be considered?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Gitian builds for commit 5d2ccf0 (master):

Gitian builds for commit 28af911 (master and this pull):

@promag

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@hebasto changed coordinates to float but result is apparently the same, so I don't think it's worth the change.

@JosuGZ I still think you should enable antialiasing only on L106 (I've tested it), this way the grid is always crisp - you can see on your screenshots that the horizontal lines are blurred and a bit darker.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@JosuGZ I still think you should enable antialiasing only on L106 (I've tested it), this way the grid is always crisp - you can see on your screenshots that the horizontal lines are blurred and a bit darker.

Good catch, I think I agree, anti-aliasing improves diagonal lines but for axis-aligned lines (and the grid is that, by definition) it can make it look blurrier for no good reason.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@JosuGZ at least please reply to the comments (even if you disagree)

@JosuGZ

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

@JosuGZ at least please reply to the comments (even if you disagree)

Hi, I haven't been on my Linux machine these days so I was waiting for that.

About horizontal and vertical lines: I think it is pointless to disable it as I don't think performance should be an issue, and sometimes lines fall between 2 pixels (if using floats, which I recall it has been mentioned). I could do it though. Same for enabling antialiasing after filling the rect with black, this one at least does not add complexity.

When I'm on it I will review a little more and I will comment.

@JosuGZ

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Do you think horizontal lines look better without antialiasing?

@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

@JosuGZ Can you please squash your commits.

@JosuGZ JosuGZ force-pushed the JosuGZ:antialiasing branch from a8533cd to db26e8e Jul 2, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

ACK db26e8e

@laanwj laanwj merged commit db26e8e into bitcoin:master Jul 2, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jul 2, 2019
db26e8e Add antialiasing to traffic graph widget (Josu Goñi)

Pull request description:

  Before:

  ![image](https://user-images.githubusercontent.com/25986871/58974389-92559c00-87c2-11e9-9673-0c47ac71de2e.png)

  After:

  ![image](https://user-images.githubusercontent.com/25986871/58974280-56223b80-87c2-11e9-8fcc-1e5d299dd1e2.png)

ACKs for top commit:
  laanwj:
    ACK db26e8e

Tree-SHA512: d795809458522a1ec19e236de30c2c5070960544162323324f0d4e2f49c3590fe7756e924f1021056106b4c52dcb564e690c15f85a15ea35342badf72653d534
@promag

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

ACK db26e8e, could you update OP screenshots?

@JosuGZ

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

ACK db26e8e, could you update OP screenshots?

Done!

Not sure if it's useful now :P

@promag

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Thanks! it can be for forks :trollface:

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.