enable relative path loading

[sebshader]

closes #10

[sebshader]

if 2 different paths resolve to the same file it will load the file twice, but it seems like pd itself does this w/ c externals.. maybe there's some lookup by absolute path that could prevent that..

[agraef]

That was a lot easier than I imagined. :) Let me give this a whirl.

[agraef]

This works, but ... it breaks the dofile and pdx live coding facilities described in https://agraef.github.io/pd-lua/tutorial/pd-lua-intro.html#dofile ff. The luatab.pd_lua file in the pdlua/tutorial/examples/luatab.pd example just never seems to be reloaded any more.

(Also check https://github.com/agraef/pd-lua/blob/master/tutorial/16-remote-control2.gif for an animated version from the tutorial.)

So your changes seem to break the dofile mechanism described there. You might not care about this, but I do. ;-) We're tantalizing close, though, so it would be a shame if we couldn't fix this. Any ideas?

[sebshader]

ah well clearly this is the issue: "NOTE: This works because the pd.Class:new():register("foo") call of the object only registers a new class if that object class doesn't exist yet; otherwise it just returns the existing class."

I changed it to register a new class every time.. this was to mimic pd's way of loading but it might be as easy as just returning existing classes again

[agraef]

Well, the filename in pdlua.c:pdlua_dofile is the right one, and the file is located correctly in pdlua/pdlua/tutorial/examples. The subsequent lua_load call works as well, otherwise it would print an error message.

I think I see what's going wrong there. Your new code in pd.lua:pd.Class:register(name) just always registers a new class now, no matter what. But it would need to check for an existing class in the case that they're exactly the same file. Can you rework it that way?

[agraef]

Yep, exactly. Sorry, our comments crossed.

[agraef]

Anyway, it shouldn't be too hard to fix this? Maybe if you first check that the class exists, then check the _scriptname member to see whether they are in fact the same?

[agraef]

And while you're at it, you should be able to remove the code for pd._check at pd.lua:47 now that it's not used in pd.Class:register(name) anymore.

[sebshader]

I think I got it. not quite sure about what's going on because I didn't look at any of what dofile is doing but it seems to work.. I don't think we need to look at _scriptname?.. (especially since 2 pd-lua classes of the same name in different directories couldn't load properly then I think)

[agraef]

You're right, that should do. Sorry, gotta run since I have to teach courses, but I'll give it another whirl later today!

[sebshader]

hmm some kinks still have to be worked out.. the 'dofile' thing is altering all of the scripts with a certain classname referenced by any path, not just the ones 'reset' is sent to. there is another little bug I found as well.

[agraef]

Well, I just ran a quick little test and dofile is working again, but it seems that the dreaded maximum object loading depth 1000 reached rears its ugly head again as well. :(

[agraef]

Yeah, this is a bit more involved after all, the internals are a bit complicated and interwoven... Well, I'll let you sort it out while I'm away teaching. ;-)

[sebshader]

@agraef between storing the object name in an object's member like _loadname, and storing it in an external array which do you think is better? On the one hand, storing the load name in the object could be useful to users/authors, on the other it would break any object already using _loadname and be a reserved member going forward.. (like _object)

[sebshader]

Ok I think I may have gotten it sorted but now I have to figure out how to deal w/ paths. for instance I can't use _scriptname because dofile loads relative to the canvas (which doesn't seem great anyways if what is wanted is to load something related to the class file) I think I might just add a 'classfile' function that searches paths relative to the class extern dir. Then the self-loading classes could use classfile instead. or maybe I just need to update the requirepaths..

[agraef]

Sounds good. I didn't see any new commits though? About the path issue with dofile (if I understand your last comment correctly), isn't that something that can be solved by just setting up pd._loadpath in pdlua_dofile just like in the loader hook?

Anyway, I had already written a reply to your previous comment, so FWIW here goes, maybe some parts of that are still useful:

@agraef between storing the object name in an object's member like _loadname, and storing it in an external array which do you think is better?

That's a good question. Technically it belongs to the class, but it needs to be somewhere where an object can easily access it on the Lua side, so I'd put it as a member into the object itself.

And no matter what name we choose for that member, there's always the risk that some existing pdlua code might break. Maybe call it __loadname stropped with two leading underscores, and if someone happens to use that, well, tough luck. ;-)

Your key idea really was to set up that global pd._loadname variable in the loader hook so that this information is readily available to the register() method in Lua land. That might be obvious in hindsight, but ingenious ideas often are. :) I wish I had thought about this myself.

We just need to add that to pdlua_dofile as well, then everything should fall into place, hopefully. And we could just always set pd._loadname (not just when a pathname is explicitly used), I think?

[agraef]

About the path issue with dofile (if I understand your last comment correctly), isn't that something that can be solved by just setting up pd._loadpath in pdlua_dofile just like in the loader hook?

Sorry, that wasn't helpful, please ignore. dofile gets invoked with the actual script name, so setting _loadname doesn't make any sense there.

Or maybe it does if you've got that _loadname member in the object now.

Guess I'll just wait and see what you come up with. :)

[sebshader]

ok I'll push what I have so far. I'm still having some trouble getting pdluax to load though. one peculiarity is that if a basename object is loaded after a pathname object, it will be the class of pathname. Really the pd loader makes this tricky bc it registers the 'new' function for both when the pathname class is loaded. I guess it might be possible to call class_new manually from some constructor if a pathname object is already using the new function (and basename hasn't been loaded) but that's getting pretty complex.. but at least for now pathname objects aren't interfering with basename objects that are already loaded.

[agraef]

Really the pd loader makes this tricky bc it registers the 'new' function for both when the pathname class is loaded.

Are you sure that this is in the Pd loader itself? Or is that defect only on the pdlua side?

I'm still having some trouble getting pdluax to load though.

For me pdluax just doesn't work at all in the latest iteration any more. Any idea how to get it working again? Otherwise it's a showstopper, I'm afraid. We can't just break pdluax or remove it entirely, that just won't fly with existing pdlua users.

[sebshader]

ohh I didn't catch that dofile returns multiple values..

[sebshader]

Really the pd loader makes this tricky bc it registers the 'new' function for both when the pathname class is loaded.

Are you sure that this is in the Pd loader itself? Or is that defect only on the pdlua side?

well, objectmaker gets methods for a pathname version and a basename version when a pathname class is loading. so it will call pathname's new function (which for pdlua is just 1 function) if you try to make a basename object, instead of loading basename's class first. but that means there has to be some way for pdlua to handle if a basename class doesn't actually exist, when pd tries to create it. ideally it would just try to load basename class and then create it. I guess it could be just keeping a string 'unregistered' or boolean 'false' or something in the class table, and then register the class before running the constructor if the value is 'unregistered' when trying to create the object..

[agraef]

Ok, that it should be an easy fix then. :)

Are you sure that this is in the Pd loader itself? Or is that defect only on the pdlua side?

I'm asking because l of this line in pd.Class:register(name):

  self._class = pd._register(name)  -- register new class

So on the C side it only ever registers the basename class, whereas on the Lua side it sets both pd._classes[name] (unless it's already registered) and pd._classes[regname]. So I think that the confusion is actually on the Lua side, not in Pd's loader.

I think that to really get all those name collisions sorted out, we need to always just use fully qualified (absolute) pathnames as indices in the pd._classes table, so that pd.Class:register(name) always finds the exact class that is being referred to via the pd._loadname mechanism. Of course, to make that work, pd._loadname will have to be an absolute path as well. This shouldn't be too hard to do, because the full path is readily available in both dofile and the loader hook.

[agraef]

Sorry, I submitted my last comment after you wrote (but before I read) yours again. :)

Ok, so that makes sense to me. Unfortunately, that makes things much more compilcated. ;-)

[sebshader]

yeah. I considered absolute path names at first, but the main difficulty is getting the correct one from the new function that pd calls, since it only calls w/ an creation name. (plus afaict that's the way pd does it for externals) I mean, it might be possible to have an intermediate array of absolute names that looks up classes. that way if 2 creation names call the same path they can just use the same class. Maybe that would be more useful/intuitive especially w/ the dofile features. (people might get confused if they reload an object that references a file but other objects that reference that file don't get updated.)

[agraef]

Ok, that seems to work now. I still think that the _loadname should be an absolute path, because otherwise you might get in trouble if you have, say different instances of foo/bar.pd_lua at different locations in the same patch.

Also, I still don't quite get why we need that separate doclassfile function. Sorry, but that seems untidy to me, and changes the existing API. Could the two be combined back into just one dofile somehow?

[agraef]

Maybe something to think about. Anyway, I have to get some sleep now (CET over here), I have to teach courses later today. I'll check back afterwards. This is in pretty good shape now already!

[sebshader]

afaict the loading system of pd is kind of all over the place. say a patch loads an external in its folder with [./objectname] any canvas that uses [./objectname] will then refer to the external from that directory, even if the folder that that canvas is loaded from has its own objectname external in it. (I only tested this once but it seems to be the case in the codes as well) So any time an object is requested by pd, it is the name that the object is created with that is registered as the creator, not the location it is loaded from. hypothetically we could store the location a file is loaded from as an absolute pathname, and just keep associating the 'creation name' it was made with with that file/absolute path. The first unique path that references the same file could be pointed at that class, but any other instances of that name will reference that file/class no matter what canvas or file they are in. (but I suppose that's the case anyways..) If there is a way to tell if a file was opened relative to parent canvas or not then hypothetically you could check if it was loaded from a different canvas and load the appropriate external/file I guess, but that seems a bit unrealistic for now imo. it might also be possible to do the 'deferred class creation' for basename objects but I'm not sure how realistic that is either.

[agraef]

yeah. I considered absolute path names at first, but the main difficulty is getting the correct one from the new function that pd calls, since it only calls w/ an creation name. (plus afaict that's the way pd does it for externals)

Ok, fair enough, then lets just keep it that way for now. It's complicated enough as it is. :)

Also, I still don't quite get why we need that separate doclassfile function. Sorry, but that seems untidy to me, and changes the existing API. Could the two be combined back into just one dofile somehow?

What I mean is: The only times that pd.Class:dofile gets called internally is in lua:in_1_load and in luax:initialize. So pd.Class:dofile might just check whether it's running inside a pdlua or pdluax class object, in which case it should call pd._dofile, and pd._doclassfile otherwise. I think that this should work.

You see, what I'm worried about here more than anything else is usability. If you give end users (and I include myself here) two different choices, dofile and doclassfile in this case, they will usually pick the wrong one. ;-) Also, I wouldn't have to change the tutorial...

Other than that, I think that this should be ready to go in now. I'll check back when I'm done with my courses today, to see whether you still made some changes. But I can also do the suggested change to pd.Class:dofile if you don't have the time.

[sebshader]

I may still try to 'fix' the issue of basenames being taken over by non-basenames

[agraef]

Ok, in the meantime here's my fix for the dofile/doclassfile issue: rev. c0bfe3d7eb6dfcc503afdb44e1d63cedab37033a

Works for me, but you may want to check it against your test examples to make sure that it doesn't break anything for you.

You can just pull that over into your branch, or I can just apply it myself after merging your PR. I just care that something like this goes in, so that we can rid of the extra user-visible doclassfile method.

[agraef]

There's still an issue with the pdluax menu_open option:

It looks like the relative script name pdlua/hello in the help patch confuses pdlua_menu_open(), but I'm not sure why. Current master is the same, though, so it's not your fault. ;-)

[sebshader]

ok, I think I got that. With all the new absolute path stuff I ended up using a class._loadpath instead of a object._loadname

I think I managed to pull out the basename objects from the pathname ones - this means that a basename will be a different class, which will be less confusing than there being different behavior for which classes are shared based on object/class creation order.

[agraef]

Ok, looking great! I'll give it a few more hours so that we can both test the latest iteration more thoroughly with our own pdlua examples, and if nothing else turns up then I can merge it later tonight (CET).

[sebshader]

huh menu_open works fine for pdluax for me on linux

[agraef]

Hmm, that's weird. Maybe some recent change in master that's not in your branch, but there's nothing related there. So it might be a platform-specific issue. I'm on Mac right now, but I can try on Linux later tonight.

In the meantime from your latest branch I'm getting this compile error:

cc -DPD -I "/Applications/Pd-0.53-1.app/Contents/Resources/src" -DUNIX -DMACOSX -I /sw/include   -DMAKE_LIB -Ilua -DLUA_USE_MACOSX -Wall -Wextra -Wshadow -Winline -Wstrict-aliasing -O3 -ffast-math -funroll-loops -fomit-frame-pointer -arch arm64 -mmacosx-version-min=10.6 -o pdlua.o -c pdlua.c
pdlua.c:502:57: error: too few arguments to function call, expected 5, have 4
            if (lua_load(__L, pdlua_reader, &reader, buf))
                ~~~~~~~~                                ^
lua/lua.h:289:16: note: 'lua_load' declared here
LUA_API int   (lua_load) (lua_State *L, lua_Reader reader, void *dt,
               ^
1 error generated.
make: *** [pdlua.o] Error 1

That looks like an editing blunder in rev. 3b94c678244cd51d43b8ba35db4902eb4e71eeee. Here's the culprit:

+#if LUA_ION_NUM        < 502
+            if (lua_load(__L, pdlua_reader, &reader, buf))
+#else // 5.2 style
+            if (lua_load(L, pdlua_reader, &reader, filename, NULL))
+#endif // LUA_VERSION_NUM      < 502

[agraef]

There's still an L where there should be an __L. I'll just do that myself and merge directly from my branch, so that I can also squash to make the history look a bit tidier. No need to keep every little fixup commit.

[agraef]

Ok, there you go: https://github.com/agraef/pd-lua/compare/master...relative_path_loader

As you can see I cleaned this up a bit by squashing all the minor corrections and splitting out the typo fix to pdluax-help.pd into its own commit. The contents is the same as your branch + the compile fixes for Lua 5.2+ I added:

diff --git a/pdlua.c b/pdlua.c
index a1559c9..21800f7 100644
--- a/pdlua.c
+++ b/pdlua.c
@@ -501,7 +501,7 @@ static t_pdlua *pdlua_new
 #if LUA_VERSION_NUM    < 502
             if (lua_load(__L, pdlua_reader, &reader, buf))
 #else // 5.2 style
-            if (lua_load(L, pdlua_reader, &reader, filename, NULL))
+            if (lua_load(__L, pdlua_reader, &reader, buf, NULL))
 #endif // LUA_VERSION_NUM      < 502
             {
                 close(fd);

If that looks good to you, I can just pull it over into master, and we consider this PR done. If it doesn't then feel free to edit as needed and force-push back into this branch so that I can merge. Thanks.

[sebshader]

seems right to me

[agraef]

Great! I already gave this a fairly thorough workout using all the examples from the tutorial. Everything seems to work fine now, but I will also check some of the other examples and other pdlua stuff I've written, just to make sure that we don't break any existing code.

I also found the issue with pdluax menu_open not working with relative paths, it's this line here (pd.lua:932):

    self._scriptname = pathname .. '/' .. atoms[1] .. ".pd_luax" -- mrpeach 20120201

That used to work when we had no support for relative pathnames, but will fail if a part of the pathname also appears as a prefix in atoms[1], resulting in duplication. We just need to use basename(atoms[1]) instead then everything will be fine. I will add that in a moment.

[agraef]

Fixed in rev. b5f53bc71e86d7c7b54ba9e709e371fd13d4047f.

[agraef]

Merged with rev. f5d0bb9c7add9125bf84610a5acfaa745c56661c. Thanks!

[sebshader]

@agraef great! I know that you already merged it, but regarding doclassfile vs dofile: I think there are situations where an external written in lua would want to reference its load location with dofile rather than its distributed location with doclassfile (like when its loading files other than itself, scripts that might be distributed w/ a patch the external is used in, or in abstraction libraries etc.)

for instance I have a library lscore and though I'm not sure I'll transition to dofile (just because I already wrote my own extension to do so in parallel with pdlua) I want users to be able to load scores written in lua that can be distributed in patches that the [lscore] object is loaded in. I could maybe do something like that with dofile. then, the composition/score could be distributed with the patch that implements the 'instruments'

so I still think it makes sense to have them separate from the user's perspective (not to mention that now dofile isn't backwards compatible afaict)

[agraef]

Sure. This is very complicated and I'm still struggling to understand the differences between dofile and doclassfile, to be honest. But thanks for pointing out your use case, it makes perfect sense to me.

So your suggestion is to revert rev. f2a889c6ab822f9049ca5bba964c675a5f7e127e?

[sebshader]

@agraef yes the difference is just where the search path looks. let's say the class.pd_lua file lives at /lib directory you have a patch that uses [class] in a different directory /patches. dofile will look for lua scripts in the /patches directory because that's where the canvas is doclassfile will load scripts from /lib directory because that is where the file resides. both have their uses imo

[agraef]

So let me try to get this straight, I really need to understand what's going on there, so that I can describe it correctly in the tutorial.

So, before we had any relative paths, the path of the class (i.e., its externdir) was just always the same as the path of the object (which is the path of the canvas with the object), so dofile would just work for both use cases.

But now we have to make sure that the class file is always loaded from its original externdir (which isn't necessarily the same as the canvas dir because of relative paths), whereas we still expect other Lua files to be loaded relative to the canvas dir (with one example being the pdlua and pdluax objects).

Did I get this right? At least it makes sense to me. :)

[agraef]

Ok, thanks for elaborating. So I think I understand this now. Joy. :)

[sebshader]

yes. tbh I'm not completely sure on the difference between the paths pd looks in for canvas_path, and using externdir but using dofile didn't work right for reloading the patch file. actually maybe now that absolute paths are used we could just load by absolute path, but then that would be up to the user using self._loadpath. by using doclassfile the user can continue loading paths like they were before (because it used to be that 'name' referred to the file path as well because it wasn't relative, but now it doesn't reflect the relative path to the canvas)

[agraef]

Ok. I still don't like the name doclassfile, though. It's an awful lot to type, but more importantly it suggests that you only use it for reloading class files, when you can actually use it to load any Lua code relative to the external (rather than the object canvas). Trying to think about a better name.

[agraef]

doxfile maybe, short for "dofile from extern dir"? Yeah, it's ugly. ;-) Do you have a better idea?

[sebshader]

hmm do_externdir? I struggled w/ that too