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

WVUMeterGL: fix initial rendering of meters invisble after skin load #11052

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 8, 2022

Fixes the initial painting issue with VU meters that are not shown visible after skin loaded.
For example, start LateNight with compact decks, the toggle the mixer: VU meters are black.

@m0dB Please take a look. 2 rendering passes fix it for me, though I'm not sure if this approach is "correct"

@github-actions github-actions bot added the ui label Nov 8, 2022
@ronso0 ronso0 added this to the 2.3.4 milestone Nov 8, 2022
bool m_bHasRendered;
// To make sure we render at least N times even when we have no signal,
// for example after showEvent()
int m_iNeedsRender;
Copy link
Member

Choose a reason for hiding this comment

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

The variable name can be improved, sounds like a boolean. We have no rule for an i prefix in Mixxx.
How about m_pendingRendings

It looks like it is a workaround for a first failing rendering. Is it reliable that the first fails and the second succeeds?
Is there any measure we can use to detect a failing attempt? This would allow to go back to the boolean and might be more reliable.

On the other hand there is no harm with this approach if it has been proven to be a solution, except the extra CPU for a pointless first render.

Copy link
Member Author

@ronso0 ronso0 Nov 9, 2022

Choose a reason for hiding this comment

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

It looks like it is a workaround for a first failing rendering.

I noticed "// 2 pendings renders, in case we have triple buffering" in one of @m0dB's PRs so I tried this. So the first rendering is not pointless but buffered and just not swapped yet IIUC.
Can we somehow figure if triple-buffering is enabled and use either 1 or 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't found a way (and honestly, the tripple buffering is an assumption, but it's the only explanation I see for what I have been seeing while debugging my QOpenGLWindow based branch). Anyway, the cost of 1 extra render is surely neglectable, so I don't think it's worth the effort.

@m0dB
Copy link
Contributor

m0dB commented Nov 9, 2022

@ronso0 , I actually used the same solution in my qopengl_renderers branch (so I should have created this PR myself, but I forgot about it!). See https://github.com/m0dB/mixxx/blob/qopengl_renderers/src/widget/wvumetergl.cpp

@daschuer I also went for an i prefix, but only because other variables in this class also use it. I don't care either way (I personally would never use type prefixes). I do think that your proposed variable name expresses better it is a counter and not a bool. (In my branch I use the similar name m_iRendersPending)

@ronso0 ronso0 marked this pull request as draft November 9, 2022 09:01
@ronso0
Copy link
Member Author

ronso0 commented Nov 9, 2022

@m0dB I adopted some changes from your branch.

@m0dB
Copy link
Contributor

m0dB commented Nov 9, 2022

Looks good to me.

@ronso0
Copy link
Member Author

ronso0 commented Nov 9, 2022

Can we somehow figure if triple-buffering is enabled and use either 1 or 2?

??

@ronso0 ronso0 marked this pull request as ready for review November 9, 2022 16:33
@ronso0
Copy link
Member Author

ronso0 commented Nov 9, 2022

Alright then, ready for merge.

@daschuer
Copy link
Member

Thank you. I can confirm it is working.

@daschuer daschuer merged commit dfa313f into mixxxdj:2.3 Nov 10, 2022
@ronso0 ronso0 deleted the vumetergl-init-paint-fix branch November 10, 2022 08:25
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 23, 2023
…aint-fix"

This reverts commit dfa313f, reversing
changes made to dba50e2.
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 23, 2023
…aint-fix"

This reverts commit dfa313f, reversing
changes made to dba50e2.
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 23, 2023
…aint-fix"

This reverts commit dfa313f, reversing
changes made to dba50e2.
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.

3 participants