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

Enable LADSPA plugins on MSVC #6973

Merged
merged 127 commits into from
May 13, 2024
Merged

Enable LADSPA plugins on MSVC #6973

merged 127 commits into from
May 13, 2024

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Nov 6, 2023

Succeeds #6758, which I messed up after a force push. Luckily, I had an almost identical branch so just merged that branch in.

@DomClark DomClark self-requested a review February 11, 2024 23:19
@tresf
Copy link
Member

tresf commented Feb 12, 2024

@tresf is doubtful about the veal changes.

I've only asked for basic regression testing. :). If you guys want to yolo it, go ahead. I'm far too busy to help with this portion right now. 🍻

@Rossmaxx
Copy link
Contributor Author

I've only asked for basic regression testing.

I don't really know what to test for over there and I'm busy with college myself. If anyone would like to volunteer, please go ahead.

plugins/LadspaEffect/caps/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/LadspaEffect/tap/CMakeLists.txt Outdated Show resolved Hide resolved
include/ladspa.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/basics.h Outdated Show resolved Hide resolved
@Rossmaxx
Copy link
Contributor Author

@DomClark what happened with the CI?

@DomClark
Copy link
Member

The CI problem was a temporary Chocolatey outage. I reran the affected jobs.

@tresf
Copy link
Member

tresf commented May 8, 2024

Are we ready to merge this?

@tresf is doubtful about the veal (Calf LADSPA) changes. Though everything else is done, other than one comment by @Veratil which can be done in a follow up pr.

Sorry for the delay. I've had some time to review the veal changes. They seem to fall into 5 categories:

  • UI changes
  • MSVC-compat
  • New features
  • Minor bug fixes/guards
  • Refactors

The only significant change was a refactor to orfanidis_eq.h, which seems to have encountered a very large refractor. At a glance, it should only affect the 5, 8, 12, 30 band EQs that Calf ships, and that's only assuming a mistake was made while refactoring.

I think the veal stuff is safe to merge.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented May 8, 2024

I think the veal stuff is safe to merge.

Can we merge #6771 then?

@tresf
Copy link
Member

tresf commented May 8, 2024

I think the veal stuff is safe to merge.

Can we merge #6771 then?

Done.

Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

LGTM though I haven't tested it

@DomClark DomClark changed the title Enable LADSPA plugins on MSVC attempt 2 Enable LADSPA plugins on MSVC May 13, 2024
@DomClark DomClark merged commit 36786dd into LMMS:master May 13, 2024
9 checks passed
@dan-giddins
Copy link

Nice one everyone 🎉

@DomClark DomClark mentioned this pull request May 13, 2024
15 tasks
@Rossmaxx Rossmaxx deleted the enable-ladspa-two branch May 18, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants