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

Improved readability and scaling of digits and avoid some artefacts #13311

Merged
merged 4 commits into from
Jun 2, 2024

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Jun 1, 2024

Based on user feedback, improved the readability of the digits and fixed the spacing and the scaling, used for the beats/time until mark counter.

BEFORE:
Screenshot 2024-06-02 at 02 41 52

AFTER:
Screenshot 2024-06-02 at 02 43 06

@m0dB m0dB force-pushed the digits-render-tweaks branch from 17f3840 to ceb923e Compare June 2, 2024 02:06
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

This PR looks good.

Now that I have tested the center mode a bit more, I noticed that the artificial font size limitations kicks in a bit early.
This as the smallest waveform I consider as reasonable, but the additional beat cont is only barely readable, the font size is ignored.
grafik
I will file an issue.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

That is strange. Before the font scaling kicks in, singleline instead of multiline should kick in first. That that doesn’t happen is the bug. It certainly does in macOS. What resolution is this? And hdpi?

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

I can't reproduce this, @daschuer . Are you sure you are running this branch?

This auto-scaling is uses

float allshader::WaveformRenderMark::getMaxHeightForText() const {
    return m_waveformRenderer->getBreadth() / 3;
} 

and if with the selected font size the text would be higher than this max height, it scaled the font size down so it fits. It will only use multiline if the twice the text height still fits in this max height.

So, if you only show beats or time, you should still get the too small font size, on a single line. Is that the case?

What happens if you return a very large number in getMaxHeightForText()?

I am wondering if maybe I am making a mistake with the device pixel ratio. Can you tell me which device pixel ratio you have?

@daschuer
Copy link
Member

daschuer commented Jun 2, 2024

My ratio is just one.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

And what happens in the same situation with only beat count?

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

Never mind, I have been able to repro the issue and wil add a solution to this PR.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

The minimum font point size is now limited to 10 pt.

I also added a 0.5 pixel margin around the text to avoid artefacts that I didn't see on HDPI, but noticed at 1:1 pixel ratio.

For reference: it is possible to run at 1:1 pixel ratio on macOS by building the bundle (cmake -DMACOS_BUNDLE=ON) and editing the Info.plist in the bundle with:

    <key>NSHighResolutionCapable</key>
    <string>False</string>

Will you do the honours and preset the merge button, @daschuer ? :-)

@daschuer
Copy link
Member

daschuer commented Jun 2, 2024

Oh sorry forgot to merge. Now the CI is failing.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

let's see now.

there is a minor issue: depending on the font size, the blurred black outline is sometimes slightly displaced from the white foreground. not a biggie though, but now that I have seen it, I can't unseen it!

@daschuer
Copy link
Member

daschuer commented Jun 2, 2024

I can confirm the minimum font size fixes the issue #13314
Are you working on a fix for last minor issue on to of this or shall I press merge?

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

Let’s wait until tomorrow. I will make a final attempt.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 2, 2024

What I tried didn't work, so I'd say merge. If I find a solution I will create a new PR.

@daschuer
Copy link
Member

daschuer commented Jun 2, 2024

Ok, thanks.

@daschuer daschuer merged commit 2221bf3 into mixxxdj:2.5 Jun 2, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants