toolkit-agnostic Core->GUI Communication - WIP

[giuliomoro]

I have done a first pass at implementing some tcl procs as discussed in #1695 to move away from passing tk commands. It is my first time at tcl and I don't have a deep understanding of the inner works of Pd, so please bare with me if anything about the style is not ideal. I #define USE_PDTK_CANVAS_PROC in any relevant file. This should be done through configure, but honestly this is easier for developing/debugging so I can switch it off/on per file as needed.

The code can be found here https://github.com/giuliomoro/pure-data-1/tree/1695-tests

I made a few NIT commits to begin with which helped me minimse the diff later on. Two commits are meant to be reverted later on as they are used only to log the actual docmds executed by the frontend in such a way that they can be used for automated testing:
a2f07d9 tcl: print docmds as they get executed. This is for testing purposes (i.e.: to produce consistent logs that are convenient to compare with pure-data-gui-tester) and should not be used in production without a switch to manually enable it
f227d05 tcl: evaluate one command at a time. This is mainly for testing purposes (i.e.: to produce consistent logs that are convenient to compare with pure-data-gui-tester and not thoroughly tested)

Throughout, I have been testing this with my pure-data-gui-testing, whose test patch has been extended to include coverage to all (I think) the tcl calls that have been affected by the commits on this branch (incl e.g.: deleting, selecting and moving objects). Where appropriate, I added workarounds in the tcl code to ensure that the tcl command being generated is exactly the same as in the code I am comparing against (a2f07d9). These can be removed at a later date.

What has been migrated:

• create, select, move, delete: plain objects, symbolatom, floatatom, listbox, commentbar, msg, bang and inlets/outlets. This doesn't include the actual text, which is created via the pdtk_text_* procedures.
• config, update: bang
• all iemguis: iemgui_label_font, iemgui_label_pos, iemgui_draw_iolets, iemgui_draw_move,
• all delete operations
• some move operations
• all text is still generated via existing - unmodified - calls to pdtk_text_*

I tried to stay reasonably close to the existing C functions. This means that I have all of the following procs on the tcl side: config create delete iemgui_label_font iemgui_label_pos move select update. This could surely be rationalised further (e.g.: config and update I think are only iemgui-specific). However in some cases I rationalised it a bit more. E.g.: text_drawborder()'s firsttime flag distinguishes between what semantically are a create and a move operation and so I made that explicit in the code.
I am minimising the number of arguments to be passed to the GUI, but still recreate the same pixel-accurate drawing. E.g.: creating a T_OBJECT passes the x1,y1,x2,y2 coordinates, creating a T_ATOM (float or symbol) passes those plus corner. In order to allow this, a minimal amount of the logic that is in the C data which is needed to draw the shapes from this data has been duplicated in tcl (see get_poly_coords()). Another example of what is needed to minimise the number of parameters passed is that only the obj's ID (ptr) is passed to the GUI in several bang methods, where it is then manipulated to generate the various OBJ, BASE, BUT, LABEL tags (see bang_get_tags()). On this note, pdgui_vmess doesn't seem to allow to pass a plain %lx which would be needed for this case, but only %p or .x%lx, so I have to trim the leading 0x from the argument in the GUI. It would be great if a type could be added that allows us to pass %lx without further processing needed on the front end.

I am currently not following the recommendation above

ideally the syntax would be closely modelled after how objects are stored on the disk.

This is in the first instance because I tried to keep the same argument order as the pdgui_vmess() calls I am replacing (weeding out unnecessary arguments), in order to facilitate my work. Furthermore, most of these calls (all except for create) do not really have a "stored on the disk" counterpart. The parameters I am passing to create include more details than what is stored to disk. E.g.: in the case above of floatatom with a -width of 5, the actual box's coordinates are computed in the backed based on font size and "width". This logic could be moved to the frontend if desired but it's not there yet. 
To complicate the matter further (at least for me), in order to send the object name's (or analogously the atom's or message's or comment's content) alongside the create() call would need some refactoring on the core side, as ::create() is called e.g.: in text_drawborder() while the text is set created in rtext_senditup(). Also keep in mind that if the call to create() looks more similar to how the object is stored on disk (e.g.: passing x0 and y0 coordinates and text), then the computation of the size of the box (rtext_width(), rtext_height()) moves to the frontend and so the hit detection problem manifests itself and needs to be dealt with.

Before doing any further work, I wanted to stop and check in first. Feel free to comment on all of this and the code: this was mostly a time-limited attempt I made in order for me to better understand the issues at hand, but I am not strongly attached to any of the code I wrote.

I thought I'd make a comment here instead of opening a PR because this is veeery far from being mergeable and this is more of a contribution to the discussion that is ongoing here, but let me know if a PR would be a better approach for this and I can do that.

Not sure whether the conversation is best continued her or on #1695

[umlaeute]

is there any specific reason to manually check the number of arguments rather than doing proc ::pdtk_canvas::create {type args} { ... }?

[giuliomoro]

not really

[umlaeute]

thanks. while it's too late here to do a full review (or even to read your lengthy text), here's some comments:

basic information about "self" on the GUI-side

i would actually prefer if the object-representation on the GUI side would know "what it is".

e.g. after issuing a ::pdtk_canvas::create "bang" $cnv $obj there's no need to keep telling the GUI that $obj should behave like a bang. instead it should just store this information in a dict.
initially i thought that we could also do away with sending the $cnv in each call, but i'm not entirely sure about this any more (i'm mostly thinking of GOP-patches that could display a single (graphical) object on multiple canvases; this is currently not possible (the GOP area greys out), but i don't see any inherent reason why this shouldn't be possible in the future.
but while i'm writing this, i think it's probably best to indeed have the $cnv (or actually: a list of all $cnv'ses where a $obj is displayed) an internal state of the GUI representation.
the only time where a GOP and a normal display need to be handled separately, is when moving the object.

[umlaeute]

GUI: widgetbehaviour

i find these switch/case like if { "bang" eq $type} {thingies (as in https://github.com/giuliomoro/pure-data-1/blob/46b6493be10f23e8f61b37e9028c421b0d89929d/tcl/pdtk_canvas.tcl#L671) unfortunate.
it doesn't scale well if we want to be able to add new widgets via externals.
it would be better to have a way to register "widgetbehaviour functions" (for common operations like move, delete or select).
we could then have e.. a single ::pdtk_canvas::move {obj dx dy} proc with a fixed signature, which then would allow us to simplify the widgetbehaviour code on the C-side (basically: ripping it out completely).

something like this (for the sake of simplicity, this misses error handling and fallbacks):

# public interface for Pd->GUI communication
proc ::pdtk_canvas::register_object {type ctor} {
   set ::pdtk_canvas::procs::constructor($type) $ctor
}
proc ::pdtk_canvas::create {type canvas obj) {
  $::pdtk_canvas::procs::constructor($type) $canvas $obj
}
proc ::pdtk_canvas::select {obj) {
  $::pdtk_canvas::procs::select($obj) $obj
}

## implementation of [bng]
proc ::pdtk_canvas::bang::create {canvas obj} {
   set ::pdtk_canvas::procs::select($obj) ::pdtk_canvas::bang::select
}
proc ::pdtk_canvas::bang::select {obj} {
   # ...
}
::pdtk_canvas::register_object "bang" ::pdtk_canvas::bang::create

[umlaeute]

namespace

personally i would go with a different namespace, e.g. ::pd::object:: - i don't really see the use of encoding the frontend toolkit ("Tcl/Tk") into the commands we send.

of course there's the precedent of what is already there, but i don't see any reason not to use nicer names for new functions...

[umlaeute]

On this note, pdgui_vmess doesn't seem to allow to pass a plain %lx which would be needed for this case, but only %p or .x%lx

this is entirely on purpose.
you should use the o type-id for sending a string identifier of any object (and i see you do that anyhow).
the GUI is then responsible for mangling this representation into whatever form it requires.
we definitely do not want to make assumptions on how a specific backend might like it.

also, the backend shouldn't need to deal with tags at all (the mere existence of a tag-concept is a frontend implementation detail).

[umlaeute]

This is in the first instance because I tried to keep the same argument order as the pdgui_vmess() calls I am replacing (weeding out unnecessary arguments), in order to facilitate my work

in the end, i think we should be more radical about replacing things. we probably do not want to end up with 3 different ways to interface with the GUI (the "traditional" one with sending raw tcl commands; a "new" one that abstracts away the gory details; and one intermediate one).

(of course, your code is just an experiment for now to see what would happen; so this doesn't really apply; i just wanted to stress it...)

[giuliomoro]

but while i'm writing this, i think it's probably best to indeed have the $cnv (or actually: a list of all $cnv'ses where a $obj is displayed) an internal state of the GUI representation.
the only time where a GOP and a normal display need to be handled separately, is when moving the object.

Sorry I don't know enough about how canvases and glist work in Pd to understand where to get the "list of all $cnv'ses", so for I have only been passing the canvas that I find in the current calls to pdgui_vmess().

we could then have e.. a single ::pdtk_canvas::move {obj dx dy} proc with a fixed signature, which then would allow us to simplify the widgetbehaviour code on the C-side (basically: ripping it out completely).

OK, I had intentionally steered away from that because the various calls I replaced with ::move have different arguments: not just in terms of abs vs rel coordinates , but also text_drawborder() when!firsttime will call ::move passing the pattern and zoom etc ... I am lumping these under ::move right now because they originate from text_displace() calls, but it could be that I am oversimplifying and they should really be something else (what? should it be create again, then the frontend should know that it should act on the existing object if the ID already exists?) My confusion also originates from me not knowing enough about when text_drawborder() will be called.

e.g. after issuing a ::pdtk_canvas::create "bang" $cnv $obj there's no need to keep telling the GUI that $obj should behave like a bang. instead it should just store this information in a dict.
but while i'm writing this, i think it's probably best to indeed have the $cnv [...] an internal state of the GUI representation.

OK, so the GUI will keep some state (currently it's entirely stateless). IIUC we should store in a dict the obj_id (passed with o in the first call) as a key and the type and cnvs as values.

also, the backend shouldn't need to deal with tags at all (the mere existence of a tag-concept is a frontend implementation detail).

Good to keep that in mind, it makes a lot of sense. Do I understand correctly then that you are suggesting we should only pass the cnv and obj_id to the frontend which should then be able to generate all the needed tags? I see a few places where this gets ugly ... For instance, for obj/*atom/listbox/msg/commentbar, (afaict) the tag is generated in rtext_new*( from canvas and then obtained via rtext_gettag() before being passed to the text_drawborder() calls. This same tag is then used as a base to generate the inlets/outlets own tags. A generic call create{type cnv id args} would be made with the same cnv and id for both an object and its iolets, but the iolets would then have to do further processing on the cnv+id combination before being stored in the dict (e.g.: set CNVID .x$cnv.t$ID; set IOLET_CNVID "$CNVID"o0). It's surely doable, but if the cnv/id combination was supposed to be unique, it is a bit ugly to call create with the same combination for several distinct objects (i.e.: the box and its iolets).

Anyhow, I think I should be able to have a go at addressing all of your observations above, but I guess I'll wait for any further observations first.

[sebshader]

I've said it before, but all of the current 'editor' functionality should be done in tcl/tk imo. There should hardly ever be a need to send a 'move' from the core to the gui; the gui should do most of that itself. It would just send 'connection' messages to pd to tell it which objects to connect or disconnect.

& the 'widgetbehavior's should also live on the gui side..

[umlaeute]

I've said it before,

we hear you.
however, "we" (a few developers in a zoom session that included @millerpuckette ) have agreed to make incremental steps.
the basic outline can be found at #1693

moving "all of the current 'editor' functionality" is summerized in workflow#6 (...).

i'm afraid, it's not high priority for now.

[Spacechild1]

I've said it before, but all of the current 'editor' functionality should be done in tcl/tk imo. There should hardly ever be a need to send a 'move' from the core to the gui; the gui should do most of that itself.

I would frame it a bit differently: there should be the possibility for any GUI backend to completely circumvent the Pd editor, especially all the collision detection. I think this has been on the roadmap as one of the end goals, but now I can't find it...

[sebshader]

@Spacechild1 well, for 'canonical' pd editor that could be the case as well.. all done in tcl/tk (maybe w/ a custom tcl/tk lib if needed). I don't see why any editor logic should be on the audio thread..

I only mention it again now bc it seemed like there was some discussion of implementing changes incrementally vs. not, but of course it's a matter of degree..

[millerpuckette]

well, tcl/tk would be a horrible language to try to write the whole editor in. Just the undo/redo stuff would be horribly difficult to write and debug in a script language. OTOH I can imagine doing hit detection up in the GUI - I think that would improve things a good deal, and it wouldn't have to imply putting any real smarts up there.
One reason to keep the gui as thin and stupid as possible is that it should make it easy to swap the thing out for something else, perhaps even something not pixel-based :)

[umlaeute]

One reason to keep the gui as thin and stupid as possible is that it should make it easy to swap the thing out for something else, perhaps even something not pixel-based :)

I think the big issue with a super-thin GUI is, that we end up with a lot of assumptions on how the GUI works in the audio core.
this makes it explicitly harder (not easier) to swap it for anything that does not meet these assumptions, e.g.

• a GUI that uses fully scalable vector graphics in order to enable zooming with more than 2 levels
• a GUI that is not "pixel-based", but instead uses a coarser raster (using Dan's "ncurses based" Pd as an example)
• a GUI that uses bitmap graphics instead of drawing primitives
• a GUI that does not have a concept of "tags"
• a GUI that does not use negative numbers for pixel-accurate font-sizes to work around rendering glitches between platforms
• a GUI that ...

so the big task at hand is to identify these assumptions and remove them from the Pd-core.

i think this will inevitably move some logic to the GUI side, thus making it "more intelligent".
It's true that this requires each and every GUI to implement this logic. but that's probably better than to first identify all the assumptions that are not met by the given GUI and implement them on top.

[Spacechild1]

@sebshader

@Spacechild1 well, for 'canonical' pd editor that could be the case as well.. all done in tcl/tk (maybe w/ a custom tcl/tk lib if needed). I don't see why any editor logic should be on the audio thread.

I did not rule out the 'canonical' Pd GUI. However, the Tcl/Tk GUI is already very slow - moving a single object on a non-trivial patch can easily max out a whole CPU core - and moving hit detection to Tcl/Tk could make the GUI almost unusable. Of course, this needs to be tested.

That's why the end goal should be about the possibility to move all (or some) of the editor logic to the GUI side. Each GUI implementation should be able to decide if it wants to implement the editor logic - or not. For example, if I were to write an alternative GUI with Qt, all the editor logic would naturally go to the GUI side because Qt's canvas implementation is extremely optimized and offers all the things we need out of the box.

[Spacechild1]

Btw, the whole roadmap currently resides in an (accidentally) closed PR (#1693). Should we rather make this a Discussion item?

[umlaeute]

However, the Tcl/Tk GUI is already very slow - moving a single object on a non-trivial patch can easily max out a whole CPU core

are you sure that this is Tcl/Tk, or the way how Pd uses Tcl/Tk?

[Spacechild1]

are you sure that this is Tcl/Tk, or the way how Pd uses Tcl/Tk?

I'm pretty sure it's because Tk's canvas implementation is not optimized and apparently does lots and lots of unnecessary redrawing... All I'm doing is moving a single object in a non-trivial patch.

[umlaeute]

All I'm doing is moving a single object in a non-trivial patch.

how non-trivial?

i just did a quick test with 10000 rectangles, 10000 ovals and 10000 text labels on a single canvas.
moving objects takes about 40-50% of a single core of my CPU (regardless whether it's a single object or all objects).

#!/usr/bin/env wish
proc make_bang {cnv tag x y} {
    $cnv create rectangle 0 0 20 20 -tags [list ${tag} ${tag}_R bang bang_R]
    $cnv create oval 2 2 18 18 -tags [list ${tag} ${tag}_C bang bang_C] -fill yellow
    $cnv create text 0 0 -text $tag -tags [list ${tag} ${tag}_T bang bang_T]
    $cnv move "${tag}" $x $y
}
foreach {::x ::y} {{} {}} {break}
proc moveme {cnv x y {tag all}} {
    if { ${::x} eq {} } {set ::x $x}
    if { ${::y} eq {} } {set ::y $y}
    $cnv move $tag [expr $x - $::x] [expr $y - $::y]
    foreach {::x ::y} [list $x $y] {break}
}

canvas .c -width 1000 -height 1000 -background white
pack .c
for {set i 0} {$i < 10240} {incr i} {
    make_bang .c "bang_${i}" [expr rand()*2000 - 1000] [expr rand()*2000 - 1000]
}
make_bang .c selected 500 500 

bind .c <Motion> [list moveme %W %x %y all]

this is not neglectible, but it's not that horrible either (i noticed no lag)

EDIT: to give more specs about my test setup. this was done with an "11th Gen Intel(R) Core(TM) i5-1145G7 @ 2.60GHz" CPU with an "Intel Corporation TigerLake-LP GT2 [Iris Xe Graphics]" gfx card (stock Xfce drivers; but properietary firmware loaded). the Tcl/Tk process was forced to a single CPU, and I tried to set it to a fixed clockrate (but miserably failed; in the end the clock rate was between 4GHz and 4.4GHz). XFce4 used another 70% of another core.

[giuliomoro]

moving objects takes about 40-50% of a single core of my CPU
this is not neglectible, but it's not that horrible either (i noticed no lag)

Wondering if it's system-dependent. It's pretty bad on my macbook pro (100% CPU and noticeable lag) using macos 10.14's Wish (tcl 8.5) or another Wish I have (tcl 8.6). This is with an Intel CPU fwiw.

Taking it down to 1000 bangs still gets 100% CPU. 100 bangs is more like 17%.

[sebshader]

@giuliomoro yes there's something suspicious w/ mac's tcl/tk implementation recently haha (see: #1488 and https://core.tcl-lang.org/tk/tktview/f642d7c0f4 that doesn't seem resolved, and doesn't seem like there's interest in resolving things..). maybe 'canonical' pd should switch gui frameworks amongst these 'big' changes..Qt or wxwidgits or juce or something
tbh I'm somewhat frustrated w/ the attitude of macos tcl/tk maintainership

[Ant1r]

I've tried the "10000 bangs" test on my "old " AMD Phenom(tm) II X4 970 (still 3.5GHz quad core) on Ubuntu 16.04.

The window takes ~8s to be fully created.
One CPU core reaches about 80% (but no more), and the lag is highly noticeable.

Maybe gfx card/driver related (my AMD CYPRESS is not supported by the AMD proprietary driver anymore, so it falls back to the open-source radeon X driver)?

[Spacechild1]

Maybe gfx card/driver related

I think it is all software rendering (which is not a problem in itself if done properly).

[umlaeute]

we might want to discuss this in a separate issue, as it seems to be unrelated to the actual PR

[giuliomoro]

superseded by #1765

[umlaeute]

is it?

[giuliomoro]

yes I think so ... anything done here that we'd want to keep should be heavily refactored to fit in with the better structured approach of #1765