MSVC: Always use Win32 ANSI functions

[messmerd]

No functional changes here, but allows building veal regardless of whether the UNICODE macro is defined

[JohannesLorenz]

Would that make sense on upstream? If yes, I would rather suggest to do it there, and then merge upstream?

[messmerd]

@JohannesLorenz I just opened a PR upstream: calf-studio-gear#362

[tresf]

@messmerd @JohannesLorenz how do you want to sync this into our branch? Commit it directly, cherry-pick it or fast forward everything from upstream? It's one of just a few remaining blockers for:

• Add Qt6 Support lmms#7339.

If fast-forward is the preference, I'd kindly ask @JohannesLorenz to do that.

[tresf]

Pinging @JohannesLorenz again so we can get this merged.

[messmerd]

It looks like the upstream repo is substantially different from this fork (since this fork is the "Last remaining LADSPA-capable fork"), so I think this PR should simply be merged without trying to incorporate any changes from upstream.

[tresf]

It looks like the upstream repo is substantially different from this fork (since this fork is the "Last remaining LADSPA-capable fork"), so I think this PR should simply be merged without trying to incorporate any changes from upstream.

This fork was explicitly created by @JohannesLorenz for the purposes of keeping up with upstream, otherwise, we'd use this snapshot in time: https://github.com/LMMS/veal/commits/ladspa-0.18/

[tresf]

Reference: #2

[JohannesLorenz]

Sorry @tresf I forgot about this. I will check now.

[JohannesLorenz]

It looks like the upstream repo is substantially different from this fork (since this fork is the "Last remaining LADSPA-capable fork"), so I think this PR should simply be merged without trying to incorporate any changes from upstream.

In the past, we intentionally always merged from calf/master (even if some older versions) into veal/ladspa.

I don't see a reason to to merge in today's master. I checked all commits since our last merge, they are

1. build system changes (not relevant to us)
2. a new effect
3. update of SF2 files
4. minor bugfixes (some of which were even made to test our Lv2 plugins)
5. MSVC/macOS compatibility

Also, this has the nice effect that we help testing calf/master. So I'm all for a normal merge calf/master -> veal/ladspa. Of course, it should be tested prior to the merge.

[tresf]

I don't see a reason to to merge in today's master. I checked all commits since our last merge, they are

So... merge this as-is?

[JohannesLorenz]

So... merge this as-is?

Depends on what you mean with "this"? 😄

[tresf]

So... merge this as-is?

Depends on what you mean with "this"? 😄

- master(this)
+ this(this);

[JohannesLorenz]

I merged and pushed it as branch merge-6d6ad781 to veal and did some basic tests. The resulting commit 789d0fa can be added to the appropriate PR for testing, and IMO this PR can be closed (sorry, and thanks anyways!).

[tresf]

I merged and pushed it as branch merge-6d6ad781 to veal and did some basic tests. The resulting commit 789d0fa can be added to the appropriate PR for testing, and IMO this PR can be closed (sorry, and thanks anyways!).

Thanks!

To be clear, you'd like to wait until testing is done before merging back into the ladspa branch, right? The qt6 branch doesn't have a lot of testing as it stands now, so I'm afraid that this is putting a Calf testing burden on an unrelated initiative.

@JohannesLorenz what's your criteria for acceptance? Do you plan on doing a bit more testing and merging or is there an expectation on people working on the qt6 branch to do this?

[JohannesLorenz]

Yes, my plan was to await testing.

what's your criteria for acceptance? Do you plan on doing a bit more testing and merging or is there an expectation on people working on the qt6 branch to do this?

I suggest:

1. LMMS build CI works on all systems
2. Simple regression tests: A user with some LMMS projects that use CALF tests if they sound the same

Given that, it might be easier to open an LMMS PR for testing, possibly even merge it to lmms/master and then merge it back to qt6?

[tresf]

Understood. I don't mind if it's against master or qt6 but I also really don't want to be the one testing this right now. Were there any issues during the merge that would suggest that there may be regressions?

[JohannesLorenz]

Nothing that suggests it, but a few quick regression tests may never harm. Build failures are more likely (though not too likely). I created #7783 - anyone can test it, if no one will test I'll merge this after a few days and inform you to update the qt6 branch. I will close this PR - thanks again.

[tresf]

Build failures are more likely

Well, your instincts were right! LMMS/lmms#7783.

Thanks for making the PR.