sdatkinson / NeuralAmpModelerPlugin

Plugin for Neural Amp Modeler
MIT License
1.99k stars 133 forks source link

std::filesystem::path is not supported in macOS<10.15 #23

Open sdatkinson opened 1 year ago

sdatkinson commented 1 year ago

Use of std::filesystem::path is preventing support for macOS versions before 10.15.

Tags: Mojave, High Sierra, 10.14, 10.13

masqutti commented 1 year ago

Did some research on this and based on https://en.cppreference.com/w/cpp/filesystem std::filesystem originally was a feature of boost library so https://www.boost.org/doc/libs/1_81_0/libs/filesystem/doc/index.htm ... Maybe include that in the project instead for backwards compatibility? Or then go with some native route. :)

sdatkinson commented 1 year ago

Might be able to solve this using WDL_Strings from iPlug2.

sdatkinson commented 1 year ago

Drawback would be that the dsp module would be locked into iPlug2, which would be bad.

Moonheadmix commented 1 year ago

Looking forward to a fix to be able to use in Mojave thank you

masqutti commented 1 year ago

Just found this which might be helpful! https://github.com/gulrak/filesystem

"An implementation of C++17 std::filesystem for C++11 /C++14/C++17/C++20 on Windows, macOS, Linux and FreeBSD."

Gordonozzy commented 1 year ago

Mojave 10.14 user—Curious if this is still in progress? Thanks

masqutti commented 1 year ago

Just found this which might be helpful! https://github.com/gulrak/filesystem

"An implementation of C++17 std::filesystem for C++11 /C++14/C++17/C++20 on Windows, macOS, Linux and FreeBSD."

According to my tests gulrak's library is a single-header with handy macro helpers and I managed to build nam plugin with it and C++14. Unfortunately it seems that iPlug2 still requires C++17 but there was just a couple of lines to fix so that built with C++14 too.

masqutti commented 1 year ago

https://github.com/masqutti/NeuralAmpModelerPlugin/commit/e3fa057f1f110b8d92dabd70d00f04a3415eed1c

Here's info how I did it, it's not a big change but I am unfortunately too occupied at the moment to do pull requests myself. It requires the https://github.com/gulrak/filesystem to exist and subfolder "Include" added as to header search paths to allow building. Also iPlug2 needs two small changes in couple of places (you will notice when you try to build) which are about Structured bindings (not possible with C++14) and very easy to fix, probably ideal to have iPlug2 issue / pull request about them since compiler only complained about 2 lines.

I've confirmed this to work on Windows 10 but not yet on MacOS.

mikeoliphant commented 1 year ago

I think it would be better to just pass paths as strings, rather than add another dependency with https://github.com/gulrak/filesystem.

From looking at the code, it isn't doing much specific to std::filesystem::path beyond some file existence checks.

masqutti commented 1 year ago

Oh yes definitely, if strings are enough. I guess I'll investigate it a bit further when I got the time 🤔

masqutti commented 1 year ago

I took a look and yes many of the paths can be just std::strings. However there are some std::filesystem methods that are required to be implemented "locally" which are:

has_filename() stem() parent_path() filename()

EchoReverbFilter commented 1 year ago

期待能够在 Mojave 中使用的修复程序,谢谢 me 2, really want try NAM on MAC 10.14 Mojave ! if that works someday, please tell me . thank u !

EchoReverbFilter commented 1 year ago

me 2, really want try NAM on MAC 10.14 Mojave ! if that works someday, please tell me . thank u !

sorry i dont know anything about coding.... im not a geek but really apreaciate ur work , hope u cant work it out ! really want try NAM on MAC 10.14 Mojave ! if that works someday, please tell me . thank u !

Vladzibor commented 1 year ago

Oh, what a pity that the plugin does not work in MOJAVE 10.14.6 Millions of users still work in this operating system. Especially in such difficult times.

masqutti commented 1 year ago

I think it would be better to just pass paths as strings, rather than add another dependency with https://github.com/gulrak/filesystem.

From looking at the code, it isn't doing much specific to std::filesystem::path beyond some file existence checks.

One idea would be to just assimilate gulrak's filesystem to this repo without a submodule as it work well as for now... :P but I don't know what harm there'd be to add a submodule as they're already used extensively in this one!

On a side-note, iPlug2 removed the only lines that made it C++17 only, which is really cool! :) ( https://github.com/iPlug2/iPlug2/commit/bc5aae0998c1cabc060a144adde56d7080d1317c )

tnyeanderson commented 6 months ago

Hey @masqutti, any chance this could be revisited? Now that the iPlug2 fix is merged, perhaps this has become more straightforward.

I tried to make a PR including the changes from your commit linked above, but there's a lot of merge conflicts and it's a bit hard to follow the history and separate what is still needed since I am unfamiliar with the codebase.

I'm willing to help where I can, and I'm happy to test on my 10.13 Mac :)

masqutti commented 6 months ago

Hey @masqutti, any chance this could be revisited? Now that the iPlug2 fix is merged, perhaps this has become more straightforward.

I tried to make a PR including the changes from your commit linked above, but there's a lot of merge conflicts and it's a bit hard to follow the history and separate what is still needed since I am unfamiliar with the codebase.

I'm willing to help where I can, and I'm happy to test on my 10.13 Mac :)

Yes iPlug removed the tiny C++17 feature and it should work, I haven't checked in for a while tho. But NAM still includes those C++17 filesystem and clamp features that prevent using NAM on older Mac OS version. clamp is an easy one, it's a simple limiting of a value inside a range, but std::filesystem features at least exists() method which checks if a file exists or not, which is required for NAM to work and thus not simple std::strings can work alone to fix this. I have forks which solved these issues and I've built NAM for down to Mac OS 10.9, but they're not up to date atm:

https://github.com/masqutti/NeuralAmpModelerPlugin/tree/dspStruct https://github.com/masqutti/NeuralAmpModelerCore/tree/dspStruct

It seems gulrak's filesystem which I've used to replace std::filesystem has been updated 2 weeks ago so it's still an active repo, to me it's still worth considering.

masqutti commented 6 months ago

@sdatkinson I can definitely still do a pull request which includes gulrak's filesystem, either as a submodule or just plain ol copy/paste ;) But I won't attempt if it's not your intention to include more repos into NAM.

tnyeanderson commented 6 months ago

Based on how many submodules are already in this repo, I think it's fair to assume that it's an acceptable method :)

I think, if it's not too inconvenient for you, getting a PR opened with an up-to-date solution to this problem will at least allow a review and give us a starting point. If changes need to be made before the PR is accepted, anyone can help out since we can all easily check the diff to see exactly what is needed to remove this dependency and allow the software to work on older systems.

Thanks for the quick response and for your hard work on this project!

sdatkinson commented 5 months ago

Thanks all for your patience on this.

I'm open to either of the two paths forward:

I think slightly prefer gulrak on account of (1) it having tests, (2) this feels like a potentially-better solution for the long run if other file manipulation comes into view that would benefit from already having a more fleshed-out implementation, and (3) the license is fine and dandy, and (4) besides, if this isn't much work, I'm sure it can be rethought in the future if need be (and keeping the std interfaces sounds nice).

So if anyone wants to PR me this I'll give it to whomever raises their hand first :)

tommyq94 commented 4 months ago

Hey everyone,

Is NAM able to load in Mojave, or will this ever be possible?

I am clueless as to coding etc, but I really wanted to try out the NAM plug-in.

Would really appreciate any help with this!

Thanks

masqutti commented 1 month ago

Thanks all for your patience on this.

I'm open to either of the two paths forward:

  • using gulrak's filesystem
  • Implementing the needed methods first-party using std:strings.

I think slightly prefer gulrak on account of (1) it having tests, (2) this feels like a potentially-better solution for the long run if other file manipulation comes into view that would benefit from already having a more fleshed-out implementation, and (3) the license is fine and dandy, and (4) besides, if this isn't much work, I'm sure it can be rethought in the future if need be (and keeping the std interfaces sounds nice).

So if anyone wants to PR me this I'll give it to whomever raises their hand first :)

I could work on this one a bit, as I already have implemented gulrak's on my repository. I wonder where should I put the submodule, in NeuralAmpModelerCore's dependencies -folder or in this repository? I noticed there's eigen in both. :o

dw199 commented 6 days ago

As a producer still working on an older system, is there a download/fork that will allow NAM to run on 10.13? Have been trying to figure out a way to use NAM.

Appreciate all the hard work that has gone into this!!

sdatkinson commented 2 days ago

@masqutti I think I'd prefer putting it here and not NeuralAmpModelerCore. It'd be easier to move it to core later than the opposite I think.

sdatkinson commented 2 days ago

@masqutti I've assigned you to this since it sounds like you shouldn't have a problem getting it done; lmk if that's ok