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

document canvas_realizedollar() #55

Closed ben-wes closed 1 month ago

ben-wes commented 1 month ago

@agraef : here's a first attempt for documenting mainly canvas_realizedollar(). currently, i only edited tutorial/pd-lua-intro.md and docs/graphics.txt (where i also added set_args()).

i'll gladly add more edits to this PR if needed, of course!

agraef commented 1 month ago

Looks good, thanks!

agraef commented 1 month ago

But why did you put the "Expanding dollar symbols" subsection into "Signals and graphics"? I think it would better fit at the end of the "Using receivers" section, because that's where those symbols will be typically used. Would you mind if I just move it there?

ben-wes commented 1 month ago

No, certainly fine for me. it does belong in the receiver context. i just put it at that later point since it's related to set_args (at least the demonstrated use case) and that was mentioned in the graphics chapter iirc.

agraef commented 1 month ago

Yeah, I can see why you did it that way, but I still moved it where it belongs now, and also reworded it a bit. I'll commit that later today.

But in the meantime I've been fighting bugs in your example and set_args itself. Did you actually test this? ;-)

First, set_args needs to be invoked with a table argument, not the symbol itself. That's easy to fix.

More seriously, set_args doesn't always seem to update the binbuf properly. That might be due to the way that you parse a string into a temp binbuf, then add that temp binbuf to the object's binbuf, after which the temp binbuf gets freed. I'm not sure whether binbuf_add really does a deep copy, so this might cause trouble with all the reallocations going on there behind the scenes while the binbuf is first cleared and then reconstructed. That's not a problem with Tim's original code since it doesn't use the temp binbuf. I'm currently looking into that.

agraef commented 1 month ago

Well, binbuf_add actually does a deep copy, and it looks alright when I print it at the end of set_args with binbuf_print. But I still find that after switching symbol arguments twice, the object still has the previous arguments when duplicated. Interesting. I'll look into that later today.

ben-wes commented 1 month ago

But in the meantime I've been fighting bugs in your example and set_args itself. Did you actually test this? ;-)

ouch ... i had started with a little more complex example here: localsendrecv.zip

... and then thought, it's enough to just show it for the sender. and then i obviously omitted the table packing of the set_args argument. sorry! i did test setting the sender, but not the binbuf writing obviously. just assumed it was fine without errors - which it isn't.

More seriously, set_args doesn't always seem to update the binbuf properly. That might be due to the way that you parse a string into a temp binbuf, then add that temp binbuf to the object's binbuf, after which the temp binbuf gets freed. I'm not sure whether binbuf_add really does a deep copy, so this might cause trouble with all the reallocations going on there behind the scenes while the binbuf is first cleared and then reconstructed. That's not a problem with Tim's original code since it doesn't use the temp binbuf. I'm currently looking into that.

understood. and curious about the outcome - although with the other example above, it looked good to me in quite a bit of testing. sorry for causing more bughunting here!

ben-wes commented 1 month ago

Well, binbuf_add actually does a deep copy, and it looks alright when I print it at the end of set_args with binbuf_print. But I still find that after switching symbol arguments twice, the object still has the previous arguments when duplicated. Interesting. I'll look into that later today.

damn, i wonder why i hadn't seen that stuff. what i see now when testing with the patch above (i do: select all, cut/paste to check the rewritten args for convenience):

... i don't see what's going on there at all unfortunately. might check tonight after a working day - but i assume that this binbuf stuff is above my head after all.

agraef commented 1 month ago

This must have been a case of pebcak, as I can't reproduce any of the binbuf wonkiness this morning.

I can see why it possibly happened, though. I was using Duplicate and then Undo to get rid of the duplicate again, but at least in Purr Data it takes two Undo commands to really undo the Duplicate -- apparently Purr Data records two different checkpoints for each Duplicate op, the first step duplicates, and the second step then displaces the duplicate. So what happened was that I actually duplicated the un-displaced duplicate (which was on top of the identical original which thus became invisible) instead of the original one, and of course clicking on the sender messages did exactly nothing to change that duplicate.

Oh boy. A serious case of user stupidity if I've ever seen one. :)) I shouldn't be debugging in the middle of the night...

Ok, with that out of the way, I can submit my tutorial changes now, stay tuned.

ben-wes commented 1 month ago

good to hear that not all is lost, haha! ... but anyway i do see 2 considerable issues after this discussion and more testing:

ben-wes commented 1 month ago

ok, it's not as bizarre as i thought, i guess ... the issue only occurs with copy/paste. and i guess what happens is simply this:

here's my updated lua file and patch for testing with some more output: localsendrecv-tests.zip

... anyway - this is probably kind of expected. and i wonder now how this would be correctly managed on the lua side?

it might be the "right" way if the initialize method gets the unexpanded symbols as creation arguments - but obviously, it would also make things more complicated (and less convenient for users who don't want to deal with this dollargs handling).

maybe @timothyschoen also has some thoughts on this?

agraef commented 1 month ago

Sorry, I can't reproduce the issue that you described with the localsendrecv-tests example that you posted. Or maybe I don't understand what the issue is. I can set both sender and receiver with the send and receive messages, and I can duplicate and copy/paste the modified object and it is created with the right (current $0-whatever) arguments exactly as I expect.

Or do you refer to the "storing \<sender> and \<receiver>" messages that get posted during initialization and then later on when updating sender and receiver with the send and receive messages?

Yes, when the duplicated or pasted object gets initialized, it will show the expanded arguments, because that's what they are during initialization, just like with the original object in the patch. The quoted $ symbols only appear when receiving send and receive messages. But that's exactly the expected behavior the way you implemented it.

If you want the unexpanded $ symbols also during initialization, that's also easy to do. But you have to quote the $ symbols in the creation arguments and when storing them with set_args. Like in the attached version of your example.

localsendrecv-tests.zip

agraef commented 1 month ago

I guess that's another one for the "\-quotes-make-my-brain-hurt" department. ;-) I'd say that everything works exactly as it should, it's just a matter of implementing the object the way you want it to be.

possibly, set_args() should throw an error if the argument isn't a table?

Agree. That's easy to add, I'll do it and then put out another release with the updated docs.

ben-wes commented 1 month ago

Sorry, I can't reproduce the issue that you described with the localsendrecv-tests example that you posted.

i'll try to illustrate it with a quick recording here:

https://github.com/user-attachments/assets/a3e5ecb8-de00-480e-8b8d-a7239d68fb25

If you want the unexpanded $ symbols also during initialization, that's also easy to do.

true - at least in these cases, where $ is the first character, this is a valid option!

Agree. That's easy to add, I'll do it and then put out another release with the updated docs.

cool. thanks!

ben-wes commented 1 month ago

following up on this with a few more notes:

i hadn't thought through this whole canvas_realizedollar() and set_args() thing. the issue demonstrated above is certainly expected: i can't rewrite an argument that i received as a creation argument since there's no way to reconstruct the unexpanded version from the expanded one.

so as soon as i'm dealing with an object that has multiple arguments where i want to manipulate one and keep another one with a dollar variable, i'm in trouble since i can't just set specific arguments. so for set_args, i need to set the whole table and have no way of putting in an unexpanded version of the one with the dollar variable.

i should have foreseen this case. :/

it's still not a bad feature - but a bit incomplete imho ...

so far, i see the following approaches to solve this:

  1. as mentioned above, initialize() could forward the unexpanded args - but i also mentioned the cons there
  2. maybe there should be another way of reading them through some get_args() method?
  3. some way of setting only specific args with set_args()

EDIT: i'd opt for 2 at the moment.

agraef commented 1 month ago

i'll try to illustrate it with a quick recording here:

Thanks, I can see it now. But the TL;DR is that I don't see anything there that can't be fixed by revising the code so that it does what you want. (NB: I think that a lot of the confusion actually arises from the fact that set_args didn't update the object text. I fixed that now, so there's no need to do the cut and paste thing anymore to see exactly what arguments have been set.)

I realize that the TL;DR may not be sufficient to quench your concerns, so let me elaborate on this. I don't see the, let's call them "effects", that you describe as bugs or deficiencies in set_arg's present implementation. Quite the contrary. I consider the API very well designed, but to get the most out of it you also need to wield some brainpower, and as a university teacher I'm always in favor of that. :smirk:

Passing string arguments to set_args through Pd's parser immediately made a lot of sense to me, that's why I merged your PR so quickly without any discussion. Because that's exactly what you get from the creation arguments in the initialize method, only that this happens outside Pd-Lua, in Pd itself. Thus the same quoting rules apply, and these are Pd's own quoting rules, rather than some ad-hoc quoting mechanism that you may devise for use with set_args.

There just remains the issue of actually quoting your arguments for set_args the Pd way if needed. But that's a problem that can easily be solved in Lua itself. I've done it in JavaScript, so I know that it can be done in Lua. Here's a bit of code I recently wrote for Purr Data which does just that, slightly massaged for presentation here -- if you squint a little, you can almost see the Lua code there.

function quote(text)
{
    var buf = "", quote = false, len = text.length;
    for (var i = 0; i < len; i++) {
        var c = text[i];
        if (quote) {
            buf += "\\"+c;
            quote = false;
        } else if (c == "," || c == ";") {
            // '\;' and '\,' need spaces around them
            buf += " \\"+c+" ";
        } else if (c == "$" && /^([0-9]|@)/.test(text.slice(i+1))) {
            buf += "\\"+c;
        } else if (c == "\\") {
            quote = 1;
            // double escape
            buf += "\\\\";
        } else {
            buf += c;
        }
    }
    return buf;
}

So I think that solving the quoting problem in Lua is very easy, and can also be adjusted for different purposes. Let's not start throwing more built-ins at something that can be solved with the language and API at hand so easily. If it turns out that something like a Lua port of the above function is used by many Pd-Lua objects then we might just add to pdx.lua or even pd.lua itself so that it's readily available.

agraef commented 1 month ago

maybe there should be another way of reading them through some get_args() method?

That also crossed my mind. But how would you implement that? The binbuf will only give you what you already have in the creation arguments, and if you use binbuf_gettext to get a quoted representation, it won't be tokenized. I don't think that this would help much.

agraef commented 1 month ago

On second though, one might write a get_args routine which takes the individual atoms and passes them through Pd's atom_string() (in m_atom.c) to construct a Lua table of all individual atoms, quoted according to Pd's rules. That would give you something that can be passed to set_args again (perhaps with some elements changed) to reconstruct the same binbuf again.

agraef commented 1 month ago

I might as well give that a go, shouldn't be too difficult since that part of Pd's API is still fresh in my mind. Stay tuned.

ben-wes commented 1 month ago

thanks for your thoughts and comments on this!

these also made me realize that one could come up with a little helper function in lua to "unexpand" (redollarize?) $0-arguments:

function myobj:dollarize(text)
  local dollarzero = self:canvas_realizedollar("$0")
  return text:gsub(dollarzero, "\\$0")
end

... but this seems rather dirty to assume that any number that equals the patch id was also originally the patch id. :) ... and obviously, this can't be easily adapted for $1 etc.

I might as well give that a go, shouldn't be too difficult since that part of Pd's API is still fresh in my mind. Stay tuned.

looking forward!

agraef commented 1 month ago

Ok, I added get_args in rev. e3927bd997aea3dd17e1904b3b63e104e217a517.

I used this method for testing, which replicated the original arguments for whatever float and symbol arguments I threw at it:

function localsend:in_1_getargs()
   local args = self:get_args()
   pd.post("getargs: " .. table.concat(args, " "))
   self:set_args(args)
   local args = self:get_args()
   pd.post("getargs: " .. table.concat(args, " "))
end

So I'd say that this works as advertised.

Please test. I'd also appreciate it if you can come up with some documentation and an example -- I can still add this before I release 0.12.18.

ben-wes commented 1 month ago

nice! and sure - i'll probably get to work on this tonight!

just a quick feedback from the first impression:

agraef commented 1 month ago

the error logs say "set_args"

Editing blunder, thanks for spotting this!

at what point can the args be accessed?

Yeah, both initialize and postinitialize get called through the Pd object instantiation callback, and apparently the binbuf gets only set up after the object has been fully instantiated. So no binbuf there yet, which means that both get_args and set_args will just bail out with no (=nil) result. I should really add an error message there if that happens.

If you really need to manipulate the arguments immediately after the object has been created, I'd suggest creating a one-shot timer in either initialize or postinitialize, that should do the trick.

i'll probably get to work on this tonight!

Take your time, there's no need to rush this. I guess that any kind of example that uses both get_args and set_args in interesting ways will need some thought. I'll just add some preliminary documentation for get_args, so that I can do a release and also update the purr-data pre-release right away. Then we can just do another release when you have something nice for the docs and/or examples.