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

improve glsl pre-roll triangles #12100

Merged
merged 14 commits into from
Oct 21, 2023
Merged

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Oct 13, 2023

draw the glsl preroll and postroll markers with a patterned texture and some minor additional cleanups

fixes #12015 , initially bringing GLSL waveforms in line with the legacy waveforms, but after discussion going for a slightly different look (closer to the legacy waveform look but with a semi-transparent filling)

before:
Screenshot 2023-10-13 at 19 49 49

initial after
Screenshot 2023-10-13 at 19 48 36

final after (with this PR)
Screenshot 2023-10-15 at 17 27 56

@m0dB m0dB changed the base branch from main to 2.4 October 13, 2023 15:12
@m0dB m0dB requested review from Be-ing, Holzhaus and Swiftb0y and removed request for Be-ing October 13, 2023 15:12
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

looks good so far, no complaints from me, except that this is just the shader code atm with the code for the pre-roll still missing.

@github-actions github-actions bot added the build label Oct 13, 2023
@m0dB
Copy link
Contributor Author

m0dB commented Oct 13, 2023

oops! pushed!

@ronso0
Copy link
Member

ronso0 commented Oct 13, 2023

could your please add before / after images to the description?

@m0dB
Copy link
Contributor Author

m0dB commented Oct 13, 2023

could your please add before / after images to the description?

done!

(i personally like the filled rectangles better btw)

@m0dB
Copy link
Contributor Author

m0dB commented Oct 13, 2023

could your please add before / after images to the description?

done!

(i personally like the filled rectangles better btw)

but maybe the something in between works best :-)

Screenshot 2023-10-13 at 19 59 37

@ronso0
Copy link
Member

ronso0 commented Oct 13, 2023

(i personally like the filled rectangles better btw)

Me too. The translucent fill is a nice compromise 👍

@fwcd fwcd added the waveform label Oct 14, 2023
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you. code looks in good shape, mostly minor suggestions. I really like the look of the translucent anti-aliased triangles. Reminds me of the Vital synth.

src/shaders/patternshader.cpp Outdated Show resolved Hide resolved
src/shaders/patternshader.cpp Outdated Show resolved Hide resolved
src/waveform/renderers/allshader/waveformrendermark.cpp Outdated Show resolved Hide resolved
src/waveform/renderers/allshader/waveformrendererpreroll.h Outdated Show resolved Hide resolved
@m0dB m0dB requested a review from Swiftb0y October 15, 2023 15:28
@m0dB
Copy link
Contributor Author

m0dB commented Oct 15, 2023

@Swiftb0y I have addressed all comments, please mark the conversations as resolved when re-reviewed.

@github-actions github-actions bot added the ui label Oct 15, 2023
@Swiftb0y
Copy link
Member

please mark the conversations as resolved when re-reviewed.

Did so and also reviewed the new code. Thank you.

@m0dB
Copy link
Contributor Author

m0dB commented Oct 16, 2023

done.

@m0dB m0dB requested a review from Swiftb0y October 16, 2023 17:55
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you, small nitpicks.

src/shaders/vinylqualityshader.cpp Outdated Show resolved Hide resolved
src/util/texture.cpp Outdated Show resolved Hide resolved
src/util/texture.cpp Outdated Show resolved Hide resolved
@m0dB m0dB requested a review from Swiftb0y October 18, 2023 20:04
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Yes, LGTM. Thank you very much. Highly appreciated. The anti-aliasing is a huuge improvement. Any objections @Holzhaus?
CI failures is unrelated, IIUC @daschuer is looking into it.

src/widget/wspinnyglsl.cpp Outdated Show resolved Hide resolved
src/widget/wspinnyglsl.cpp Outdated Show resolved Hide resolved
m0dB and others added 2 commits October 20, 2023 01:23
@JoergAtGithub
Copy link
Member

I just tested this PR with vinyl control signal and the spinnies show it without color (just black):
grafik

@m0dB
Copy link
Contributor Author

m0dB commented Oct 20, 2023

@JoergAtGithub Aargh, the only thing I forgot to test myself! Thanks, I will fix this now.

@m0dB
Copy link
Contributor Author

m0dB commented Oct 20, 2023

@JoergAtGithub I have fixed the vinyl quality shader. If you confirm it works, this is ready for merge.

@Holzhaus
Copy link
Member

Yes, LGTM. Thank you very much. Highly appreciated. The anti-aliasing is a huuge improvement. Any objections @Holzhaus?

Looks nice. My only objection regarding the previous look was that Software/GL/GLSL/Shader waveform rendering should be consistent. Is this the case now.

@m0dB
Copy link
Contributor Author

m0dB commented Oct 20, 2023

With 50% transparency, it is 50% more consistent ;-)

At least it is now clearly deliberate and can't be mistaken for a bug. So IMO it is "consistent" enough and sacrificing a bit of consistency for a nicer look makes sense.

We could try to change the legacy waveforms to match this look, but I don't think that's worth the effort, particularly taking into account that there is already a lot of variation between the different waveform types.

@JoergAtGithub
Copy link
Member

The color of the Vinylcontrol signal is now back.

I'm not sure if the following is a regression, or was always the case, but when I drop the needle, I see the signal outside of the round area:

Aufzeichnung.2023-10-20.232434.mp4

Maybe it's just me, but I expected this to be masked as the Cover-Art.

@m0dB
Copy link
Contributor Author

m0dB commented Oct 21, 2023

Maybe it's just me, but I expected this to be masked as the Cover-Art.

That would be as simple as swapping the draw order of the mask and the vinyl quality. But I took this order from the original spinny code, and changing this should not be part of this PR.

Merge?

@JoergAtGithub
Copy link
Member

Since it's not a regression, let's merge this as it is!

@Swiftb0y
Copy link
Member

Thank for this nice visual improvement @m0dB. Thank you for the thorough testing @JoergAtGithub.

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.

7 participants