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

waveformseekbar: remove hires configuration and add hidpi detection #2261

Merged
merged 3 commits into from Mar 1, 2017

Conversation

@ptitjes
Copy link
Collaborator

@ptitjes ptitjes commented Feb 22, 2017

Fixes #2258.

@declension declension self-requested a review Feb 24, 2017
@declension
Copy link
Member

@declension declension commented Feb 24, 2017

Thanks. This however breaks rendering behaviour for quiet sections on all waveforms:

ql-2261-2017-02-24-quiet-waveform

vs current master:
ql-master-2017-02-24-quiet-waveform

@@ -218,8 +224,8 @@ def draw_waveform(self, cr, width, height, elapsed_color, remaining_color):
if u1 != u2 else 0.5)

# Draw single line, ensuring 1px minimum
cr.move_to(x, half_height - val)
Copy link
Member

@declension declension Feb 24, 2017

Losing this 1px behaviour probably causes the problems rendering.

Copy link
Collaborator Author

@ptitjes ptitjes Feb 24, 2017

Indeed. I'll fix that !

@@ -190,23 +191,28 @@ def reset(self, rms_vals, player):
self.queue_draw()

def draw_waveform(self, cr, width, height, elapsed_color, remaining_color):
line_width = CONFIG.line_width
hidpi = self.get_scale_factor() == 2
Copy link
Member

@declension declension Feb 24, 2017

This should probably be more lenient for future even-higher DPI (or non-OSX hidpi)
e.g. > 2 or maybe > 1 even, not sure

Copy link
Collaborator Author

@ptitjes ptitjes Feb 24, 2017

I have no idea how GTK people will make this (integer) property evolve in the future. I'll test for > 1 then.

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 24, 2017

@declension So I made the modification relative to the use of the value returned by get_scale_factor().

However I am not able to reproduce your 1px problem...
Would you mind rechecking with this latest patch ?

Here's what the unstable version from lazca repository draws:
unstable

Here's what this PR draws in non-HiDPI mode:
patched

And here's what this PR draws in HiDPI mode:
patched-hidpi

@declension
Copy link
Member

@declension declension commented Feb 24, 2017

Can't see any change here:
waveform-2261-2017-02-24-2

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 24, 2017

@declension Would you mind trying again, please ?


# Draw single line, ensuring 1px minimum
cr.move_to(x, half_height - val)
cr.line_to(x, ceil(half_height + val))
Copy link
Member

@declension declension Feb 24, 2017

What was wrong with the ceil? That works for me

Copy link
Collaborator Author

@ptitjes ptitjes Feb 24, 2017

The thing is that doing a ceil draws a line that is at least one pixel high. Say val=.25, you will draw a line of 1.25px high.

(BTW I highly suspect the reason of the anti-aliasing being disabled in the code has been made for this reason. I think it could be removed once the drawing code has been cleaned.)

Not even to speak that if you have a resolution higher than the coordinate system (HiDPI, printing, ...), then using ceil completely fails !

@declension
Copy link
Member

@declension declension commented Feb 24, 2017

Also, note: it looks like you're force-pushing rewritten commits. Now that this is public that can make life harder for people (re-)pulling, and loses the history of changes you're doing to the PR (i.e. Commits: 1 is a lie).

Github allows the merger to squash these commits if it suits the project, so perhaps it's best to push normal commits - with commit message to match the intent of the change.

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 24, 2017

I am sorry if that shocks you. I am used to do that for such a tiny PR and as is done in a patch/mail workflow. It enables you to see the difference to the actual code and not to my last diff. If it causes problems to you, I can proceed your way.

I only have HiDPI displays. So I need your help to test my patch on non-HiDPI devices so that it only brings enhancements to the current way. Also, I think that adding conditionals to the drawing code would be a shame if we can find a cleaner way to do it whatever the display resolution is. So I intend to try some more tweaks, if you will to help me test those.

@declension
Copy link
Member

@declension declension commented Feb 24, 2017

@ptitjes ok. I don't have much time left here but have tested your latest code. Perhaps a VM would help here.
Anyway I see no real change:
waveform-2261-2017-02-24-2

By adding 0.5 and -0.5 on (your) lines 229,230: a "fatter but symmetric zero" approach but probably my favourite, given that it's a seek bar too and there's no additional highlighting (EDIT: sorry, I'd also increased the total height - try to ignore that)
2261-waveform-2017-02-24 18-55-57

Or just removing the AA disabling:
pr-2261-2017-02-24 18-28-27

As you can see it allows the sub-pixel precision, but it looks pretty bad (which is why I disabled it)

* draw in the middle of pixel to draw sharp lines (c.f. Cairo's FAQ)
* set anti-aliasing back
* apply the same rendering to the placeholder
@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 24, 2017

@declension OK. We happen to have different tastes :) But I think I found information in Cairo's FAQ that explains the problem of non-sharpness of the zero line.

I modified my code accordingly. I think it can fit both of us. Please tell me what you feel.

@declension
Copy link
Member

@declension declension commented Feb 24, 2017

Thanks - looks better vertically yes but still blurred:
2261-waveform-2017-02-24 20-13-13

Useful Cairo link, thanks; Adding 0.5 to the horizontal fixed the remaining (horizontal) AA problem... 😄

2261-waveform-0 5px-2017-02-24 20-14-27

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 24, 2017

@declension On the line_to and move_to lines ?

* draw horizontally in the middle of pixel to draw sharp lines
@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 25, 2017

@declension OK, so my last commit should now handle that, whatever the DPI scale factor is.

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 26, 2017

If/When this PR gets accepted, I've got those two following commits to PR:

ptitjes@0f26f84: waveformseeker: enhance redraw performance and shorten interval

  • add a mechanism to update TimeTracker's interval
  • enhance waveform redraw by using Cairo's clip rectangle
  • shorten redraw interval to make the position travel smoothly

ptitjes@4756bda: waveformseeker: use separate time trackers for label and waveform

Fixes #2255.

  • use separate time trackers for label and waveform
  • do less updates depending on the incoming events

@declension
Copy link
Member

@declension declension commented Mar 1, 2017

LGTM, though can only test on HiDPI currently:

ql-2261-waveform-2017-03-01

Aside: we should probably scale the height of this in HiDPI actually, looks pretty small relative to the text

@declension declension merged commit 9a2b601 into quodlibet:master Mar 1, 2017
1 check passed
@declension
Copy link
Member

@declension declension commented Mar 1, 2017

Thanks

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 2, 2017

Thank you.

@ptitjes ptitjes deleted the hidpiwaveform branch Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants