agraef / pd-lua

Lua bindings for Pd, updated for Lua 5.3+
https://agraef.github.io/pd-lua/
GNU General Public License v2.0
51 stars 11 forks source link

doesn't load objects using relative paths like ./objectname #10

Closed sebshader closed 1 year ago

sebshader commented 3 years ago

when creating an object I can't use ./myobject, like I can for abstractions and externals

porres commented 1 year ago

it's not only that, you can search for objects in subfolders wither, like [pdlua/hello], so there's something bad and maybe old in the loader mechanism

porres commented 1 year ago

trying to debug this.

say I have the [hello] object in a /pdlua subfolder. We need [declare -path pdlua] otherwise it won't work. If you just try [hello], it won't work, because it is in the subfolder, but if you try to call it as [padlua/hello], it fails as well. Now, here's the weird part, if I try to create it again as [hello], it finds it! If I try [pdlua/hello] again, it doesn't work again.

This indicates that the object is found by Pd when trying the '/pdlua' prefix and it is remembered when trying just [hello].

Anyway, there's a clear bug here we should try and fix. By using the debug functions in the object I get:

pdlua_new: s->s_name is pdlua/hello pdlua_new: start with stack top 0 pdlua_pushatomtable: stack top 3 pdlua_pushatomtable: end. stack top 4 pdlua_new: before lua_pcall(L, 2, 1, 0) stack top 4 pdlua_new: done lua_pcall(L, 2, 1, 0) stack top 2 pdlua_new: done FALSE lua_islightuserdata(L, -1)

This means it fails here where it should be TRUE --> https://github.com/agraef/pd-lua/blob/master/pdlua.c#L466

porres commented 1 year ago

if we check pd's console with "verbose" checked in preferences/path, we can see that Pd does find 'pdlua/hello' easily. It just can't load it.

Screen Shot 2023-01-08 at 02 14 53

note we even get it to print 'hello universe'!

porres commented 1 year ago

hmmm, I see where it fails now. Pd loads the file alright and 'hello universe' is printed... the fail happens inside the hello.pd_lua file right next, in

local Hello = pd.Class:new():register("hello")

If I change it to

local Hello = pd.Class:new():register("pdlua/hello")

then it works when I create the object as [pdlua/hello].

agraef commented 1 year ago

Should be fixed now. Please test, thanks.

sebshader commented 1 year ago

@agraef seems like it isn't working here quite the same yet.. if I do [../luaobj] in some subdirectory it doesn't create the object from the parent directory (compiled with luajit5.1 debian bullseye) whereas c externals and abstractions do create that way however [subdir/luaobj] and [./luaojb] both work fine now

agraef commented 1 year ago

Lua 5.1 is very old, I'm surprised that the current version of pd-lua works with that at all. (5.2 might still work, but 5.3+ is what I'd recommend.)

That said, I'm having issues with .. as well. Sometimes it works, sometimes it fails with this mysterious message:

maximum object loading depth 1000 reached
../pdlua/hello
... couldn't create

Looks like the vanilla loader is getting confused by the initial .. and recurses. I'm not sure whether pd-lua is to blame here, or the loader. Will have to look into it.

sebshader commented 1 year ago

well like I said, it works for binary externals and abstractions so it seems like there is something pdlua could do.. (I get the same error btw) I like to use 5.1 both because of JIT and because setfenv and getfenv allow a more efficient/less hacky implementation of dynamic variables for an environment to make music with, so you could have one function use the caller's scale or tempo or something. (don't have to iterate to get the _ENV upvalues of a function using the debug library, or pass in an environment as an argument, though maybe I'll end up going w/ the latter anyway if some kind of 'scoped macro'/HOF can be used easily..).

anyways, would it make sense to re-open until a solution is found?

agraef commented 1 year ago

Yes, I'm reopening because my implementation isn't really working in some corner cases.

The actual issue is that a pd_lua file registers its class name by itself, and this is just the basename, without any path. So, you can't just load, say, ./hello.pd_lua and ../hello.pd_lua both implementing the same hello class. With Pd abstractions this kind of thing works, but it's basically impossible with the way pd-lua is currently designed.

The situation is really similar to what happens if you have two external binary objects with the same name under two different paths. Pd will then print a warning and just load one of the instances. At least that's how I remember it.

So the pdlua loader should do something similar. To do that, on the C side I need to check if a class with the same basename has already been registered on the Lua side, in which case it should print a warning and just instantiate the existing Lua object class instead.

sebshader commented 1 year ago

@agraef I believe w/ pd it is possible to load different binaries by path. e.g. if I use [lib1/classname] and [lib2/classname] it will result in 2 different classes.

agraef commented 1 year ago

in which case it should print a warning and just instantiate the existing Lua object class instead.

Unfortunately, this doesn't work either. The loader is a harsh mistress. :( It's either create a new class, or bail out with failure. Everything else leaves the loader in an unstable state in which it recurses and crashes.

Oh well, then so be it. I can still check for class name collisions and bail out with a sensible error message. that will at least fix that ominous "maximum object loading depth" message. This seems to be the way to go.

sebshader commented 1 year ago

image different help files open here so I think it's working

agraef commented 1 year ago

Ok, the name clashes should at least be properly handled with rev. e28dc6dae5050de954bbaf8aeed88c3916c05732 now.

I believe w/ pd it is possible to load different binaries by path.

Ah ok, that's nice. Unfortunately, I can't make pdlua work that way without redesigning its object class system. I have some ideas how to do this, but this will be a considerable amount of work and need a substantial amount of testing, so this will have to wait for another day. If anyone wants to beat me to it, please go ahead and send me your PR. ;-)

sebshader commented 1 year ago

well, it works now but now I can't load the same class with multiple different paths/from different directories. it would be nice if at least instead of 'class already exists' and just giving an error it could just load the already-loaded class if it exists in the correct location imo I mean really the entire point is to avoid namespace clashes between objects of the same name so I guess it doesn't matter right now. so maybe an error/ non-creation is better after all... maybe I'll try to figure out how all this fits together and take a crack at it..

my use-case is that I use pdlua for an object [lmap] in my library (to wrap lua's arrays) that I want to load from a patch in a subdirectory 'manual'. So I was using [../lmap] to make sure the 'manual' patch would load the right class and open up the correct helpfile w/o clashing with other libraries' objects

agraef commented 1 year ago

it would be nice if at least that instead of 'class already exists' and just giving an error it could just load the already-loaded class if it exists in the correct location imo

Yes, I understand, but there's no way to do it as far as I can tell. (If you know a way, send me a PR please.)

The proper solution is to overhaul pdlua's object class system so that foo/hello and bar/hello would be different classes, which isn't possible with the way it's currently designed. This is certainly the goal and why I keep this ticket open.

sebshader commented 1 year ago

@agraef ok I gave it a shot (I just added a _loadname field to the pd table when loading a non-basename, then added it to pd._classes in the register function)

agraef commented 1 year ago

Fixed now (for real this time) by @sebshader's PR, thanks!

agraef commented 1 year ago

With that it's time for another release, but I'd say I'll give it another day or two so that people can test it and we can shake out any remaining bugs.