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

Bump veal submodule #6771

Merged
merged 2 commits into from
May 8, 2024
Merged

Bump veal submodule #6771

merged 2 commits into from
May 8, 2024

Conversation

tresf
Copy link
Member

@tresf tresf commented Jul 12, 2023

  • Adds MSVC support to "Calf" plugins.
  • Adds several upstream patches

See also LMMS/veal#9, LMMS/veal#5, LMMS/veal#6, (etc)

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Jul 12, 2023

Have found some issues. Old project files where i've used the calf plugins seem to crash. Maybe cause of missing upgrade mappings.
I happened to mess up on the building, no such issues when i rebuilt clean. Also, old files load fine, not much issues. Haven't tested indepth though. If someone could test it better, would be appreciated.

@tresf
Copy link
Member Author

tresf commented Jul 13, 2023

old files load fine, not much issues. Haven't tested indepth though. If someone could test it better, would be appreciated.

Thanks for testing. I realize now that quite a bit has happened on our fork, this adds more than just MSVC support. We can merge it and cross our fingers or wait on some detailed testing.

@tresf
Copy link
Member Author

tresf commented Jul 13, 2023

This may be superseded by #6758

@Rossmaxx
Copy link
Contributor

feel free to merge this before #6758. That will make it easier to debug calf related issues in the future and keep that pr for enabling support alone, bumping can be done by this pr instead.

@tresf
Copy link
Member Author

tresf commented Jul 13, 2023

If it were just a matter of MSVC changes, I'd merge this without question, but since there were several fast-forwards from upstream code changes, it increases the chance of a regression. I can merge this, but I will not have time to jump into tracking down regressions, so this is a risk. I just want another developer to OK this.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Aug 15, 2023

Happened to take a look at the code again and saw that the majority changes are, in fact, msvc changes. Also, most of the other changes look harmless too. The only suspect possible of causing any regression is in the orfanidis_eq.h file. I wanted to test deeper but college is interfering pretty hard so can't do anything other than looking at the diff on GitHub via mobile.

Edit : took another look and found that the remainder of the changes were either version bumps, xml changes or gui related (which we don't currently use as we have our own). Also about the orfanidis_eq.h file. There was a refactor in that file. Don't think that might cause any regressions or issues.

Inviting @zonkmachine for testing.

@JohannesLorenz
Copy link
Contributor

From a code perspective, no complaints against this (but testing would not harm).

Note that in the recent days, I merged a few commits to calf's master, and there may follow some in the next week, as I am setting up a CMake base CI for it, including MSVC support. Maybe it is better to still wait? But I am OK with merging, too.

@messmerd
Copy link
Member

messmerd commented Dec 16, 2023

The veal submodule has 3 new commits since this PR was opened, so they could be included too.

Though if #7015 is merged first, that won't be needed.

@tresf tresf merged commit acefd06 into LMMS:master May 8, 2024
9 checks passed
@tresf tresf deleted the veal-msvc branch May 8, 2024 05:06
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.

4 participants