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

fix an error and improve ELSE posting #23

Closed porres closed 1 year ago

agraef commented 1 year ago

if defined(ELSE) || defined(PLUGDATA)

I don't see the rationale for this. Right now, pdlua in plugdata is separate from ELSE, so the "part of ELSE" message isn't warranted. If this changes and the separate pd-lua submodule is removed, and pd-lua will be compiled as part of ELSE, then the message will appear anyway, and rightfully so.

Ok, I'll just commit the other source change manually.

agraef commented 1 year ago

See, I just want the message to be accurate, otherwise it becomes confusing -- the user might think he's running the pdlua version from ELSE, when actually he's running the upstream version, and these two are not necessary indentical (and indeed they aren't at the moment).

porres commented 1 year ago

I don't see the rationale for this. Right now, pdlua in plugdata is separate from ELSE, so the "part of ELSE" message isn't warranted. If this changes and the separate pd-lua submodule is removed, and pd-lua will be compiled as part of ELSE, then the message will appear anyway, and rightfully so.

PlugData ships with ELSE, and it's carrying [pdlua] not on its own but as part of the ELSE library. So I do believe that when people use this external in PlugData that they should be aware it is not an external object from plugdata, from another library incorporated into it or whatever. They would know that it is coming from or also available in ELSE. What do you think @timothyschoen?

Moreover, when posting for ELSE, I don't think it is relevant to add information when this was compiled as I already have a post for this when loading ELSE about it's release date (and library version and what Pd version is required).

See, I just want the message to be accurate, otherwise it becomes confusing -- the user might think he's running the pdlua version from ELSE,

what do you mean, that the version carried in PlugData is from upstream and not paired up with ELSE?

timothyschoen commented 1 year ago

what do you mean, that the version carried in PlugData is from upstream and not paired up with ELSE?

I think we both include the submodule, so we should be in sync right? If not, I would like to be in sync with ELSE, so we should fix that.

Regarding the startup message:

If you don't put the "Distributed as part of ELSE" message underneath it, ELSE users might be confused about why they're getting init messages from pd-lua. It might be nice to add that line, but I get that there's kind of a conflict of interest, since pd-lua already contains things that are specific to purr-data and plugdata, so if we keep doing that it will become messy.

In plugdata, these messages will be muted anyway, so it doesn't matter. Since pd-lua is now a part of ELSE, it can be debated whether we still need the pd-lua init message in plugdata.

porres commented 1 year ago

@timothyschoen , currently we already post a message when the external is loaded as part of ELSE, see

https://github.com/agraef/pd-lua/blob/master/pdlua.c#L1708

The thing is if we should also post the same in plugdata

In plugdata, these messages will be muted anyway

well, that settles it then :) cause it's pointless. I will just send a new PR with a simple modification on how I want the posting to be by also removing compiling information because it it redundanct for me

porres commented 1 year ago

but then I gotta say I think it makes sense to post messages on PlugData (I don't really use it), let me get it straight, all information printed in the terminal gets ignored? That doesn't seem right and I guess I misunderstood what you said.

agraef commented 1 year ago

I think we both include the submodule, so we should be in sync right?

Not necessarily. That requires a concerted effort, because those submodule hashes don't magically advance on their own. Also, there are some minor differences (mainly the ELSE branding in the toplevel pdlua help patches).

If not, I would like to be in sync with ELSE, so we should fix that.

That poses a problem for me, because that would effectively block me from contributing further advancements in the plugdata-pdlua interface. Right now I can just update to the latest from my upstream and submit that along with related changes on the plugdata side, like I have done before. If the pdlua in plugdata is the one from ELSE, then I can't just test any new changes without first submitting a PR to the ELSE repo, which is impractical.

I do care about this, and you should, too, because there's at least one remaining major issue on my TODO list, namely making pdlua multi-instance capable. If you want me to work with you on this, then we better keep the upstream pd-lua submodule for now so that I can actually, you know, work on this.

Once this is finished and plugdata's pdlua goes into maintenance mode, you can just go ahead and reorganize the sources all you want, and slap ELSE all over them. ;-)

If you don't put the "Distributed as part of ELSE" message underneath it, ELSE users might be confused about why they're getting init messages from pd-lua.

I fully agree with that, but we already have that #ifdef ELSE in upstream for that prupose, so it's properly displayed in vanilla if you're running the pdlua from ELSE. I just refuse to conflate #ifdef ELSE with #ifdef PLUGDATA in the upstream source, as was proposed in this PR, because this is technically incorrect and misleading -- the two versions could be very different.

In plugdata, these messages will be muted anyway, so it doesn't matter.

That's true. So let's stop beating this dead horse. ;-)

porres commented 1 year ago

That's true. So let's stop beating this dead horse. ;-)

Yeah, I could confirm in personal chat with Tim. I will just send a new PR as what I wanted doesn't really make sense.

timothyschoen commented 1 year ago

That poses a problem for me, because that would effectively block me from contributing further advancements in the plugdata-pdlua interface. Right now I can just update to the latest from my upstream and submit that along with related changes on the plugdata side, like I have done before. If the pdlua in plugdata is the one from ELSE, then I can't just test any new changes without first submitting a PR to the ELSE repo, which is impractical.

Okay, that's cool, then I will leave it the way it is. The extra init message for pd-lua then also shows that it's actually not the same one that's in ELSE, and it will still lead to compatible patches.

agraef commented 1 year ago

Yeah, I could confirm in personal chat with Tim. I will just send a new PR as what I wanted doesn't really make sense.

Are we talking about the plugdata side here? I actually like the concise information that plugdata displays about installed externals, but currently these are hardcoded into the plugdata source.

There are potential ways to communicate proper live version information about bundled externals in the setup routine. I'm already using a custom setup routine for pd-lua in plugdata which could be extended that way, and a similar approach would also work for cyclone and ELSE. That way plugdata could display accurate, up-to-date, and concise version information about the included externals, without having to rely on those awfully verbose sign-on messages.

porres commented 1 year ago

That way plugdata could display accurate, up-to-date, and concise version information about the included externals

I think that'd be nice

agraef commented 1 year ago

In case you're wondering how that would work, the custom setup routine (on the condition of an #ifdef PLUGDATA) would simply take an extra char[] buffer provided by plugdata (along with the size of that buffer) into which the external can then write any live version information as it sees fit.

E.g., for pd-lua one might include the pd-lua and lua version numbers, optional ELSE branding, along with the git revision and last tag. This information can all be determined automatically at compile time using compiler directives and something like the output from $(shell git describe --tags | sed 's/\([^-]*\)-g/r\1/;s/-/./g') in the Makefile.

Here's how that might hypothetically look like: pd-lua 0.10.2.r29261c00c (ELSE), Lua 5.4. This might be a little over the top, but if someone reports a bug with that information then at least you know exactly what he's running. ;-)

Of course, the same information could also be used to generate the sign-on message for plain old Pd.

The downside of the git describe method is, of course, that it only works in the git source. But it shouldn't be too hard to find a fallback that would work with release tarballs.