agraef / pd-lua

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

add self._canvaspath field to objects #28

Closed sebshader closed 1 year ago

sebshader commented 1 year ago

this way, classes could load assets other than lua scripts from the canvas path (and ._loadpath already exists)

agraef commented 1 year ago

This is interesting... I always assumed that the cwd would be set to something sensible when opening a patch, maybe the program directory, maybe even the canvas dir, who knows? I now checked this with a little pdlua script, and as it turns out, in vanilla the cwd is /, I wonder why that is? In Purr Data it's actually the user's home directory which seems to be a little more sensible. (This is on the Mac, on Linux or Windows this might well be different, I haven't checked.)

So yes, that proves your point that something like _canvaspath is actually needed. ;-)

However, wouldn't you expect that _canvaspath is already defined when self:initialize() is being called? Currently it's set after initialize() has been run, which is too late if you'd like to load your data files during object initialization. self._object gets set as soon as pd.Class:construct starts executing, so it shouldn't hurt if you move the definition of _canvaspath before the self:initialize() call, no?

sebshader commented 1 year ago

good point, for some reason I was thinking it should go between initialize and postinitialize but that is worse for a couple reasons.

sebshader commented 1 year ago

actually maybe it would also be good to include a utility function to get an ancestor's directory as well. (so you could specify getcanvaspath(2) to get the directory of the grandparent canvas) now we have [pdcontrol] but it might be more convenient to not have to wrap classes in an abstraction, especially since then you'd also have to pass the abstraction's arguments manually

This is often useful when writing abstractions. I have some code from my library I could port over

agraef commented 1 year ago

Yeah, I forgot about postinitialize, but I agree that this is better.

agraef commented 1 year ago

You should maybe add a little demonstration. Here's a minimal example based on the "hello" object which I used for testing, but which might also serve illustration purposes.

hellocanvas.zip


pd.post("Hello, universe!")

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

function Hello:initialize(name, atoms)
  pd.post("Hello, world! canvaspath = " .. self._canvaspath)
  self.inlets = 1 
  return true
end

function Hello:finalize()
  pd.post("Bye bye, world! canvaspath = " .. self._canvaspath)
end

function Hello:in_1_bang()
  pd.post("Hello, bang! canvaspath = " .. self._canvaspath)
  return true
end

This could go into pdlua/examples. I'm sure you can come up with something more interesting, though. ;-)

agraef commented 1 year ago

actually maybe it would also be good to include a utility function to get an ancestor's directory as well.

Hmm, no, I have to disagree. I think that the external should be free from general utilities and concentrate on the object interface itself. Also, this is a slippery slope where you end up with a grab bag of random things, because everybody has their own favorite Lua tools they want to work with. ;-) That's what luarocks is for, it has hundreds if not thousands of modules which you can use (and ship with your app if needed).

sebshader commented 1 year ago

hmm I suppose the only time it would be relevant is if it was being used in an abstraction, in which case you could pass a path from [pdcontrol] anyways..

I guess for a good example I might make some subdirectory in the examples folder, and load a .pd_lua file from that subdirectory as an abstraction that processes some text file in the examples directory, where the parent patch lives

agraef commented 1 year ago

I guess for a good example I might make some subdirectory in the examples folder, and load a .pd_lua file from that subdirectory as an abstraction that processes some text file in the examples directory, where the parent patch lives

Better keep it simple, don't make it a full score editor. ;-)

I think it's better to have the example patch and pd_lua in examples, since that's how the examples are generally organized and where people will look for your example. And have the text file etc. in a subdir, so that people don't get confused with the auxiliary materials if they don't care about that example.

Or just put the entire thing into its own subdir of examples, that would work as well.

agraef commented 1 year ago

OT: Looks like I added the CI builds just in time, so that I didn't have to pull your branch in order to test it out. (In case you didn't notice, the builds for your PR can now be accessed on the Actions tab, offering ready-made Ubuntu, Mac, and Windows packages for each commit.)

sebshader commented 1 year ago

also slightly OT: I'm thinking the absolute path setup right now isn't working. the same class isn't being referenced because if you load an object w/ [./obj] vs [obj] the path would be /my/absolute/path/./obj vs /my/absolute/path/obj so we have to find some algorithm or regex to split off the relative and current-directory 'dot' specificiers

agraef commented 1 year ago

Yeah, we could normalize the paths, but that seems over-kill to me. Let's call it a case of pebkac (user gets what s/he asked for). It's not a bug, it's a feature! 😜

agraef commented 1 year ago

Ok, I'll check back later today, and as soon as you added the example, I will go ahead and merge.

sebshader commented 1 year ago

well at least it seems good to take care of/add the trailing path separator, so that people can just concatenate "_loadpath" or "_classpath" with either _scriptname or another file. I hope I'm doing it right for windows too.. the separators that pd gives might have to be replaced or something

agraef commented 1 year ago

The slashes work just fine on Windows, too. Here's a beefed up version of my hellocanvas example which includes a text file located in a subdir. The bang now uses _canvaspath to load that text file and post its contents in the Pd console. It works the same on both Mac and Windows, and I'm 100% sure it'll work on Linux as well although I haven't tested that.

hellocanvas.zip


pd.post("Hello, universe!")

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

function Hello:initialize(name, atoms)
  pd.post("Hello, world! canvaspath = " .. self._canvaspath)
  self.inlets = 1 
  return true
end

function Hello:finalize()
  pd.post("Bye bye, world! canvaspath = " .. self._canvaspath)
end

function Hello:in_1_bang()
  local fname = self._canvaspath .. "/hellocanvas/hellocanvas.txt"
  local f = assert(io.open(fname, "rb"))
  local content = f:read("*all")
  f:close()
  pd.post("Hello, bang! " .. content)
  return true
end
agraef commented 1 year ago

Here's proof that it works:

grafik

agraef commented 1 year ago

I can just take that example and add it to your PR. That will give you time to think about a better example which you can submit at a later time in a separate PR if you wish.

sebshader commented 1 year ago

ok I attempted a help patch and also added trailing slashes to _canvaspath and _loadpath for convenience/consistency

agraef commented 1 year ago

Looks perfect to me! Apart from the 'holp patch' typo in the help patch, that is :), but I can still fix that in post.

agraef commented 1 year ago

Merged, thanks! I'll fix that typo in the help patch on master, and then release 0.11.3 immediately, because (1) it's such an important feature, and (2) it gives me the opportunity to test my new gh-release automation. ;-)

agraef commented 1 year ago

Typo fixed in rev. 12a6fb0283cdb85b90165f571f5a5b79dafb4e96.

agraef commented 1 year ago

Finally figured out how to fix up my gh-release config, so after some hours of trial and error and hitting my head against the wall the new release is out now. ;-) But in the future getting releases out of the door will be much quicker now.

sebshader commented 1 year ago

@agraef great. btw on the release page it says '_canvasdir' but the name is actually '_canvaspath'

agraef commented 1 year ago

Fixed (in the release notes, not the commit msg, which I can't change anymore in order not to disturb the release tag).

agraef commented 1 year ago

@sebshader, I'm afraid that you'll have to do something about the fallout of the trailing '/' change in rev. a4621af43ad0de7b03eedb4b0dc970e628779517 in this PR.

This breaks the reloading of class files (dofile, dofilex), because those trailing slashes keep piling up. E.g., /foo/luatab then becomes /foo//luatab, which of course means that it won't find the existing class in the pd._classes table, which breaks the dofile/dofilex mechanism. Which is really bad. :((

Here's a quick change which works around the issue:

diff --git a/pd.lua b/pd.lua
index 8dfe9e3..78573ef 100644
--- a/pd.lua
+++ b/pd.lua
@@ -243,8 +243,13 @@ pd.Class = pd.Prototype:new()
 function pd.Class:register(name)
   -- if already registered, return existing
   local regname
-  local fullpath = pd._loadpath .. '/'
+  -- Those trailing slashes keep piling up, thus we need to check for whether
+  -- pd._loadpath already has a trailing / here. This is only a temporary
+  -- kludge until a proper fix is deployed. -ag 2023-02-02
+  local fullpath = string.sub(pd._loadpath, -1) == "/" and pd._loadpath or
+     (pd._loadpath .. '/')
   local fullname = fullpath .. name

   if nil ~= pd._classes[fullname] then
     return pd._classes[fullname]

I'll release this as a hotfix in version 0.11.5 later today. But I think that there ought to be a better solution to this issue. Can you please look into this and see what you can do?

sebshader commented 1 year ago

Ok I'll look at it in ~10 hours

agraef commented 1 year ago

Thanks. No need to hurry -- for the time being, 0.11.5 is out with the workaround I described.

sebshader commented 1 year ago

maybe we could do

  -- remove trailing / from loadpath
  pd._loadpath = self._loadpath:sub(1, -2)

in dofile and dofilex?

agraef commented 1 year ago

Have you actually tested that this works? But then again, your kludge doesn't seem to be much better than mine. ;-) In fact, I'd say that my work-around has the advantage that it will catch all cases where loadpath already has a trailing slash, no matter what the actual call sequence leading to pd.Class:register(name) happens to be.

agraef commented 1 year ago

Please try 0.11.5. It works for the test cases I have, if it also works for you, then we can maybe just keep it as it is.

sebshader commented 1 year ago

It seems like it's doing the trick (at least I'm not seeing duplicate // in the _loadpath)

my thinking was that before register is called, pd._loadpath should be the same as what it was when pd set it (to be consistent). That way there don't have to be as many branches and argument handling: the 'caller' is responsible (if we consider pd._loadpath as an argument) but hey, whatever works works

agraef commented 1 year ago

Ok, got it. Yes, it works either way, callee or callers, and there are good reasons to prefer either solution. Extra-cautious programmers might even do both. If you care about it enough, you're welcome to send me a PR. Otherwise, let's just keep it as it is. ;-)