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

Shade: Audio Latency meter fix #11601

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Shade: Audio Latency meter fix #11601

merged 4 commits into from
Oct 5, 2023

Conversation

daschuer
Copy link
Member

This fixes an issue with the GLSL Shader when the widget has only on pixel height.
Simply by using two widgets.

I have moved a +1 to various places in our code, but I was not able to make it work. Only if the Widget itself has a height of two pixels, it works.

@m0dB do you have an idea how to fix it?
Can you confirm the issue with macOS?

@m0dB
Copy link
Contributor

m0dB commented May 29, 2023

I confirm that on macOS the vumeter doesn't show either when the height is 1. The problem is not in the shader code, I tried by just simply making the window red with glClearColor and returning. With 2 pixels there is no problem (note btw that the size is determined by the background pixmap). Looks like a Qt bug to me.

I could implement a workaround: when the height is 1, make it two and set the glViewport to 1 pixel height.

Alternatively, I think it would be much better of the overload formed part of the vumeter, instead of using a separate statuslight for this. That would also solve the problem.

@daschuer
Copy link
Member Author

Thank you for confirming that it is nothing wrong in your code. Maybe one with a more recent QT version can confirm if it has been fixed with QT 5.15.

A new widget that combined both indicators would be nice. However it affects the skins. Not suitable for a beta. It would be welcome for main though.

Is the glViewport workaround easy enough or should we just merge this workaround here?

@m0dB
Copy link
Contributor

m0dB commented May 30, 2023

I think the glViewport workaround would be easy enough but still it would be a bit of a hack and it would make the code more dirty (it would not just be calling glViewport, but also conditionally clearing the background transparently I think), so I think your solution of using a 2 pixel high image in the skin is a better option. (I didn't check the png you provided, but if the bottom row is transparent, it should like exactly the same as before)

I would however add a check in the code so at least we show a warning when a 1-pixel height (or width I guess) is being created, so skin creators don't lose time figuring out why some widget is not being shown.

On a side note, when looking the skin xml, I noticed the tooltip of the audio latency vu meter is wrong.

@daschuer
Copy link
Member Author

The tooltip is already fixed here: #11598

I had no luck with a transparent line in the background image, because black is shining through not gray.
I will verify it again and also add the skin warning.

@m0dB
Copy link
Contributor

m0dB commented May 30, 2023

Hm, if the transparency shows black, this indicates that the background color is not determined correctly by

m_qBgColor = mixxx::widgethelper::findBaseColor(this);

If you confirm this is the case, I can debug that.

@daschuer
Copy link
Member Author

With the border halve transparent and 200 % it looks like this:
grafik

@daschuer
Copy link
Member Author

With QOPENGL=OFF
grafik

@daschuer
Copy link
Member Author

And finally with --disable-vumetergl
grafik

@daschuer
Copy link
Member Author

The issue is that in shade the background is a png, not a brush. The dark gray color is the backround color used before the background png is rendered. I don't think there is a big demand to fix that, we just need to look for another workaround here.

@daschuer
Copy link
Member Author

Using the background color instead of transparent works:
grafik

@daschuer
Copy link
Member Author

Now it looks exactly like before.
I have the fix for #11600 in my stash as well and added it here.

@daschuer
Copy link
Member Author

Superseded by #11722

@daschuer daschuer closed this Jul 28, 2023
@daschuer
Copy link
Member Author

daschuer commented Oct 5, 2023

I can confirm that this fixes the Shade issues on Ubuntu Jammy as well.
@m0dB should we merge this and carry on?

@m0dB m0dB merged commit 19d80b5 into mixxxdj:2.4 Oct 5, 2023
11 checks passed
@m0dB
Copy link
Contributor

m0dB commented Oct 5, 2023

LGTM! Merged. Thanks for taking care of this!

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