Multiple paint layers #57
Conversation
|
Here's the hello-gui example with multi-layer drawing. Unfortunately, github doesn't accept .pd_lua files |
Btw, after merging in the latest changes, I get this error in plugdata. Any idea why? Seems like the code is only passing one string to _setrequirepath while the lua code expects 2 arguments, or maybe I messed something up while merging. |
|
cool! tested this with vanilla here - works perfectly well. can't respond to the |
I remember that I added the extra pdlua_path argument at some point in 0.12, but that's long gone again. So it looks like your pd-lua source is in an inconsistent state, and you're running an outdated pd.lua there. There have been lots of changes in the entire source since you last merged, so make sure that you get all of it. :) |
|
Two things I noticed:
|
|
Also, what about the repaint method, can it still be invoked without arguments as before? Otherwise it would break a lot of existing scripts, so I assume that the answer is yes. ;-) |
|
Other than that, the changeset looks good to me, I just need a little time for the purr-data port. |
|
Also, Ben, thanks for testing the vanilla variant, it's good to know that this works, since it means that probably all I'll have to do is to massage the JavaScript code! |
I'll do that! Probably put that in at some point to get it to compile, before merging your changes.
Yeah, the plugdata-side implementation for pdlua imports the "pdlua.h" file, and "class" is a reserved keyword in C++, so it wouldn't compile. By changing the to "pdlua_class", the header is valid C and C++ code. Now I'm writing this though, I realise that I should have done "extern 'C'" instead, so I guess we could revert it. |
|
just quickly adding to this: Pd crashes with some of my graphical objects now. i'll investigate more later what the relevant difference there is between the gui example and my objects! |
If it works, please do. Otherwise the name of the shadow class needs to be changed accordingly. I despise inconsistent naming, it makes code harder to read.
That's not good, please report it here so that Tim can fix it. Pd-Lua gfx code should never crash Pd, no matter how broken it is. |
|
@timothyschoen one finding concerning the crash: it's obviously connected to a clocked |
yep - tim mentioned that above: |
Unfortunately, it didn't work. Bascially any name that isn't "class" would be okay for me.
Gonna check now why that is! |
Ah, that's probably it. Thanks! |
|
I think f8e94a3 fixes the crash. The indexing for layers was a bit messy, since I kept the 1-indexing from lua in the C code, which also made it allocate space for one more layer than we need. This messy logic led me to make a mistake that caused the crash. Should be all good now. Btw: comparing the hello-gui example in pd-vanilla with and without layers makes a huge difference over here. With layers, it easily stays at 60fps, without layers the framerate drops pretty low |
|
cool, thanks! for some reason, i now get errors when redrawing the first or all layers in a little test patch: ... your new hello-gui.pd_lua looks good though and also the previous version works again. i currently don't see what i'm doing wrong. just using this test: local t = pd.Class:new():register("gui-test")
function t:initialize(sel, atoms)
self.inlets = 1
self.delay_time = 50
self:set_size(200, 200)
return true
end
function t:postinitialize()
self.clock = pd.Clock:new():register(self, "tick")
self.clock:delay(self.delay_time)
end
function t:finalize()
self.clock:destruct()
end
function t:paint(g)
local b = math.random()*255
g:set_color(b, b, b);
g:fill_all();
end
function t:paint_layer_2(g)
local b = math.random()*255
g:set_color(b, b, b);
g:fill_rect(1, 1, 99, 99);
end
function t:tick()
self:repaint(1)
self.clock:delay(self.delay_time)
endEDIT: it's still drawing the 1st layer then - but with errors. if i do just |
|
ah, i see now that a @timothyschoen in this context: i guess, the syntax for the layers is intuitive and analogous to the inlet messages - i'm just wondering if |
Ah, that makes sense, since I did change the logic for that. Must've made a mistake somewhere. I'll take look, should be fixable! |
Yeah, that's because it's a reserved keyword in C++. Then keep it the way it is, but please also change Looking at OK, I'll wait until you fixed the teething problems before I have a go at porting all the gui changes. Note to self:
(Ben, you could maybe give me a helping hand with any of these items if you have the time.) |
|
Should be fixed! |
can confirm that the warning is gone and it works now for a single gui object instance! if i load a patch with 2 instances, one is somehow "stealing" the first layer though. this also happens when i'm redrawing the first layer of one: it's disappearing for the other object then. cutting one of the objects then also gives me: |
You're right. Inertia and laziness prevented that so far. :) There's also some other files that should go there, have a look at the definition of pdx_files in the toplevel pd-lua Makefile. (At least pd-remote.pd will be needed for the live-coding support.) |
|
Ok, I pulled your branch into my working copy, and started a deep dive into the source now, in order to figure out what exactly I need to change in the PURR_DATA part. There's still some stuff that I need to understand a little better, it would be nice if you could help me out there. (Sorry if you already provided that information, but this thread is getting a bit long, and I don't have the time to dig through it again.)
I think that it's for now, but I'll let you know if I run into other problems. |
|
Note that by "really absolutely 100% sure" I mean "could produce a mathematical proof of the fact if I needed to". In all other cases defensive programming is in order! 😏 Better safe than sorry, IMHO. |
|
Another question: Where is the memory of the layer tags (both the char * pointers and the array itself) freed again? I don't see anything in the code. |
No problem!
Layer tag 0 should get initialised and used. It's associated with the regular paint(g) call. It's at least used when we call pdlua_gfx_clear, to clear the first layer when it needs to be updated. It also gets used in end_paint to do ordering. Note that we get the layer index as
The call to pdlua_start_paint is made in pd.lua, so we can control the values that go in there. So it shouldn't be able to go wrong. If you want to add an extra check in case the user incorrectly modifies pd.lua, that's fine with me.
I see what you mean, I thought I made sure that can't happen but looks like it can happen! So we need to fix that. Leaving holes is not allowed currently, but it also shouldn't crash because of it. I've just made a change to pd.lua that should make sure this can't happen (and also makes it easier to read), but if you want to be extra sure, we can also update the C code to make sure it won't crash. I think it should never happen, but the safer the better. |
Oops! I still need to do that |
|
Looks to me like transforms can also potentially leak, since it only gets freed when we reset them in start_paint. I guess we need a pdlua_gfx_free function to clean up the layers and transforms. |
|
I fixed the memory leaks, hope it's not too much of a hassle for you to merge in. Also confirmed with leak detection that we don't leak anything anymore. |
|
Random thought, but perhaps we should heap allocate the pdlua_gfx object, so that it doesn't take up memory for non-gui objects? Not a priority right now I guess. |
|
Sorry for the many commits btw, the draft was less complete than I had hoped it was! |
|
No worries, my changes are confined to the PURR_DATA parts (at least so far), so I can just rebase on your latest and be done with it. (Also feel free to force-push if needed, I know how to deal with that, no problem.) |
Ok, I see what you did there in 696d227, but it's not helping, sorry. :) Let me elaborate. Your proof of correctness rests on the assumption that pd.Class:repaint(0) is called before anything else so that the entire layer tags array gets populated there, and that's initially true. But that assumption breaks down with live-coding, just as it did before, and your commit doesn't help with this. Consider this use case: The user starts out with a simple animation with just one layer, layer 1. The object is created with this layer, at which point pd.Class:repaint(0) gets called and the tags table is populated with one entry. (Or maybe 2? I still don't quite get your numbering scheme there, I'm afraid.) Then the user adds two extra layers, layer 2 and 3, and adds messages to kick off those parts of the animation using separate clocks. He reloads the script, which will not cause pd.Class:repaint(0) to be called again, because the object is already instantiated and running. You can not prevent this from happening, because the raison d'être of live-coding is precisely that the underlying Lua code can be swapped out at any time without any disruptive state changes in already running instances of the object. While the animation keeps running, subsequent pd.Class:repaint() calls will pick up the new layers 2 and 3 and, as they are consecutive, will add them both to the paint_funcs table. But their entries in the layer tags array on the C side still haven't been created yet, because pd.Class:repaint(0) didn't run (and never will again in this example). But now the user, let's call him Albert D'Evil, adds the requisite messages for the layer 2 and 3 animations to his patch and decides that he wants to kick off the layer 3 animation first, because ... why not? :)) So pd.Class:repaint(3) is called and there you got your hole at index 2 in the tag table now. As soon as the user then kicks off the layer 2 animation, Pd will crash and burn. You see, with live coding the whole environment becomes a whole lot more dynamic, so all your assumptions about the execution sequence go out of the window, no matter how hard you try. The only way to fix this problem is to cope with the out-of-order layer initializations on the C side when populating the tags table. And it's very easy to do, just do the snprintf thing right there in in start_paint() with all newly created layer tags instead of just a single one. I also don't like that you're now constructing and checking the entire paint_funcs table each time pd.Class:repaint() is called, even if it's just to update a single layer. That might become a performance hog if you have many layers, because in an animation that method might be called many times per second, and it will then have to reconstruct the entire layer functions table each time. So it kind of defeats the purpose of having layers which can be updated independently in the first place. TL;DR: As 696d227 doesn't really help with the out-of-order layer tag creation problem, and it introduces substantial overhead for no real gain, I'd revert it, and implement the proper incremental initialization of the tags table in start_paint() that I sketched out above instead. Problem solved. :) Of course, I can do it for you if I didn't explain it clearly enough. |
|
One more thing: You're using resizebytes() to resize the layer tags array in start_paint(), but malloc() for the initial allocation, and free() for deallocation in pdlua_gfx_free(). Which might be inconsequential if Pd's underlying implementation in fact uses malloc/realloc/free. But in any case it's inconsistent -- please use either malloc/realloc/free or their Pd equivalents, I think that's safer. |
Ah, I hadn't considered that. Makes sense, gonna work on it! I'll also revert the pd.lua change.
I'm not so sure, because when this happens in end_paint: It will try to position layer 3 on top of layer 2, but tcl/tk is not aware layer 2 exists yet, so it will print an error. So we need to find a way around that too. |
|
The layer indices in Lua should be: 0: Not a real layer, but will trigger a repaint of all layers When we convert that into C, we subtract 1 so that we can use it as an index for the layers array, so: Hope that helps, let me know if you spot anywhere I did it wrong. |
|
That last problem is kind of a fundamental one. We can't render on top of layer 2 if it doesn't exist. I see 2 possible solutions: Solution 1: We create an invisible dummy tag for skipped layers, so we can order things correctly. We'll need to correctly position the dummy layer, and after that the real layer relative to that. IMO: this feels bad because now we are in a state where layer 3 is visible but layer 2 is not. I don't feel like that should be possible. I also feel like this would be a lot of code just to make something work that's not actually valid. Solution 2: If we notice a hole in the paint function chain, we trigger a full repaint. We consider it an invalid state, and solve it by re-rendering everything. I've verified that this works perfectly and solves all our problems, but it feels pretty nasty code-wise, because we're synchronously repainting all the layers, inside a layer repaint call: For now, I'm going with solution 2, because I feel like this actually solves the problem instead of trying to make an invalid state work. But feel free to work on this if you have the time. |
This reverts commit 696d227. # Conflicts: # pd.lua
No, it makes perfect sense now, thanks for explaining it once more and sorry for my slow-wittedness. :)
Right, I hadn't thought about this bit, because that Of course, with the layer implementation, I will now have to deal with that kind of situation in the PURR_DATA code, too. But I can actually solve this by simply adding the SVG equivalent of what you called a skipped layer (solution 1) right there in start_paint(). In HTML/SVG this is just an empty SVG group which may be filled with graphics objects later if and when the layer actually gets painted. This makes it possible to maintain the correct stacking order without having to resort to a full repaint. It would be awesome if something like this could be implemented in vanilla's Tcl/Tk GUI as well, but I can see why you're running into toolkit limitations with this. :( While I still like Tcl/Tk for its simplicity, it shows its age there. Having the DOM and HTML+SVG rendering engine of a modern HTML browser at your fingertips makes all this much easier. Ok, I think that I can continue with the purr-data port now. I haven't got very far with it yet, but I think that I have a good grasp of the layer architecture now, so this should be easy to do. |
|
Ok, I think that this will do: d0dd9a7...496b965 Great job, Tim, as always! Porting this for purr-data was surprisingly easy, congrats on the excellent design! For the record: In the purr-data implementation, this just required adding the layer management in start_paint, and adjusting pdlua_gfx_clear accordingly. The graphics operations are virtually unchanged, except that they now take the layer tag in lieu of the object tag as argument. And in the purr-data pdgui.js module, I just needed to add a single new gui_luagfx_new_layer function which takes care of creating the layers (a.k.a. svg 'g' objects) which act as containers for the svg graphics objects now. And this should be entirely backward-compatible, just like it is in vanilla -- with older pd-lua versions, it will just continue to use the gobj as parent instead of the layers and never call gui_luagfx_new_layer at all. At least that's what I hope. :) I still need to test this, which I'll do over the weekend. In particular, I need to make sure that the generated layer tags don't collide with tags we already use in the purr-data GUI, but that seems unlikely. Alright, I'm going to merge this now! |
|
All done and dusted, 0.12.19 is tagged and currently building. I still had to fix a few glitches and updated the docs, so Tim, please make sure that you merge back the latest into your fork. |
Draft for multi-layer painting for pdlua.
Currently, it works like this:
Apart from your normal paint function, you can define extra draw callbacks that draw on top of it:
Then, you can repaint the layers separately by calling
The primary motivation is performance. If you have a static background with moving foreground, it won't have to repaint everything all the time.
Currently implemented for pd-vanilla and plugdata, so purr-data still needs to be done. I might even have broken the purr-data implementation a bit. So far it seems to work well, but the pd-vanilla version could use some more testing.