jpcima / ysfx

Hosting library for JSFX
Apache License 2.0
161 stars 26 forks source link

Hi, this is really amazing! #7

Closed ghost closed 2 years ago

ghost commented 2 years ago

I just built the vst3 which worked fine (linux) the only step i found missing was the submodules which i got with git submodule update --init --recursive if you want to add it to the readme.

As probably expected some jsfx load and work, some don't. Are you interested in testing feedback already or is it too early in the process? Anyway very exciting!

jpcima commented 2 years ago

Yes please, send all the feedback, this would be much appreciated.

ghost commented 2 years ago

Cool so here is a very quick unorganized look (ignoring gfx):

https://github.com/JoepVanlier/JSFX/tree/master/Squashman

[ysfx] error: Squashman.jsfx: cannot find import: squashman_upsamplers.jsfx-inc
[ysfx] error: ???: no source is loaded, cannot compile

dropdown

https://github.com/JoepVanlier/JSFX/blob/master/Basics/MS-20.jsfx

So far for now, like I said just a quick look. If you know all this already please let me know (before I spam your issue tracker with known stuff :) )

jpcima commented 2 years ago

Any jsfx that look for files in other directories don't load

I can't repro this one at all, what is your setup?

jsfx that don't have a desc: field don't load (but that's a descision you made i think?)

That field is specified as required, I was supposing that all jsfx must have it. Do you mind to link one of these files?

jpcima commented 2 years ago

with the then following dropdowns being ignored

The warning here is inconsequent, the reality is that following sliders have a syntax problem in them. As you see, it has 2 opening < characters. slider6:1<0,1,1<{Off,On}>Inertia

Regardless, thanks for noting, i'll make the parser more permissive.

ghost commented 2 years ago

You cannot repro means it works for you? I don't have a special setup, all jsfx go in .config/Reaper/Effects (imported via reapack but that doesn't change the hierarchy)

I think the specs are pretty lose at various points, you can do all sorts of things "wrong" and it still works (inside reaper, which is what I compared it to) which is of course good and bad, but also means that's how a lot of jsfx look like :)

example that doesn't use desc: https://github.com/leafac/reaper/blob/main/Effects/leafac_Bit%20crusher.jsfx (but you can also just delete that line from any jsfx and load it in reaper)

jpcima commented 2 years ago

I think the specs are pretty lose at various points

Yes that's what I figured, and I already identified some of the cases, and fixed a few from jsusfx. It happens that the spec is sometimes plain wrong, checking against Reaper is a better reference.

Anyway I've fixed MS20 at least, and added a unit test for that; we can improve the test suite with cases like that as we go, it's also the reason why i've made it.

You cannot repro means it works for you?

Yes it doesn't give that problem you refer to. You're loading via plugin right ?

ghost commented 2 years ago

yes with the plugin. i just tried again with the squashman example and it is the same message. cannot find import: squashman_upsamplers.jsfx-inc which is in the Dependencies folder.

jpcima commented 2 years ago

The spec again mentions desc as required field, even as marker of jsfx file type to use for detection. Unfortunately, Reaper is thinking otherwise. It gets also in the way of how discovery is done in Carla, so that will be to revise as well.

jpcima commented 2 years ago

cannot find import: squashman_upsamplers.jsfx-inc which is in the Dependencies folder.

Right I understand what's the deal here; whatever import files it doesn't find, it scans for them recursively in the Effects folder.

ghost commented 2 years ago

I think the hard requirement in reaper is the @sample block and a somewhat correct math. It's perfectly fine to add a few more requirements for the plugin, that won't be a deal breaker :)

jpcima commented 2 years ago

@sample definitely is not required. midi plugins don't use this

ghost commented 2 years ago

see not even that

ghost commented 2 years ago

Great! I went through the Effects folder here, now all of them load and display all sliders, the remaining errors I get are all gfx related, really cool. Audio is not always there but I make that a separate issue.

jpcima commented 2 years ago

the remaining errors I get are all gfx related, really cool.

Ok gfx is implemented as stubs, these errors should go away

ghost commented 2 years ago

Adding, because I just stumbled over it: jsfx's import method (in reaper) is case insensitive. example: https://github.com/JoepVanlier/JSFX/blob/master/Yutani/Saike_FMFilter2.jsfx#L116 doesn't load in ysfx because of a case mismatch. (sorry @JoepVanlier for using you as the example again :))

JoepVanlier commented 2 years ago

Hahaha, no worries! Thanks for pointing these things out @mynameismuhl . I'll do fixups on my end for this stuff as well. A lot of it comes from kind of sloppy coding practices when doing jsfx to be honest. That and no linter being available. Personally, I wouldn't mind a stricter implementation :D

Thanks @jpcima for setting all of this up. I frequently get requests from people who want to run some plugins in other DAWs on Linux and your implementation could be a great solution for them :)

jpcima commented 2 years ago

jsfx's import method (in reaper) is case insensitive.

ah, yes, rather unfortunate, but true. I'll make this its own issue.

You're welcome @JoepVanlier , and your work has been helpful so far.

jpcima commented 2 years ago

doesn't load in ysfx because of a case mismatch.

This one should be fine from now.

ghost commented 2 years ago

Since everything in here is either solved or in a separate issue I am closing this.