glist_getcanvas should climb up to the ancestor canvas on which we're to be drawn, then return that canvas. By removing gobj_shouldvis, that is exactly what it does.

glist_getcanvas should not find bounding boxes, nor check if objects should be drawn. That is pointless and expensive work.

Consider the following made up code to draw a [bng] object:

gui_vmess("gui_draw_bng", "xxii", glist_getcanvas(x->x_glist), tag, xx, yy);

With my code the meaning of glist_getcanvas is obvious. We return the canvas on which the [bng] is to be drawn.

The old way, glist_getcanvas would do one of two things. If the widget is to be drawn, it will do the same as above. However, if it's not to be drawn, it will return a child canvas below the canvas on which it is not to be drawn. That just doesn't make any sense. I'm not sure how that wasn't causing errors in Pd-extended since trying to configure a non-existent tk canvas would result in an error.

In other words-- if you've already hit glist_getcanvas, you should already be sure the relevant object should be visible. That's the way Pd Vanilla currently works, too.

glist_getcanvas should not find bounding boxes, nor check if objects should be drawn. That is pointless and expensive work.

Well, that makes sense to me. I also see that this call isn't in vanilla either. All I can say that this causes some gop updates to just never get done, or in the case of my patch no gop updates being done at all. Which is fixed by re-adding the call. So something isn't right in Purr Data there. Looks like there's a call to that function missing somewhere else then.

Oh well, I guess that I'll have to go through the tedium of trying to boil my patch down to a minimal example, so that you can reproduce the bug. That'll take a while, it's a complicated patch. But this is a really serious issue for me -- it basically makes Purr Data unusable for me right now. So for now I'm just re-adding the call in the JGU packages until this gets fixed.

One thing you could do is go through the zip files on this issue and see if they all work without crash/error in your new branch:

#390 (comment 3966)

I think one of the crashers was an infinite loop, but unfortunately I didn't document where it was happening in the git log.

Also-- I had some success "bisecting" the test patches in the zip files the user sent me for the last regression.

Basically, just deleting half the objects, seeing if the crash occurs, then repeating until getting down to the least number of objects. (Also, toggling the "-noloadbang" flag and testing various canvas properties through that process.)

FWIW, here's the upstream commit in which the call to gobj_shouldvis got removed: https://github.com/pure-data/pure-data/commit/ae4e9dc64e7e4281aed8228ed49cc2c6c8932549. So this crash bug apparently got fixed upstream a long time ago, but for some reason that fix never made it into Pd-extended or Pd-l2ork.

I see some accompanying changes to g_editor.c in the upstream changeset, I'm currently trying to pull these over and see whether that has any effect on my gop update issue.

One issue with this code is that it has diverged quite a bit from vanilla. Leaving aside the (unavoidable) changes for the nw.js port, there's some strange kludge in the glist_doreload routine where it calls canvas_create_editor(gl) to work around cases where canvas_vis doesn't quite do the right thing. I guess that this is responsible for some of the flashing of subpatch windows going on when saving an abstraction which occurs several time in a patch. I wonder if it's possible to get rid of this?

I see some accompanying changes to g_editor.c in the upstream changeset, I'm currently trying to pull these over and see whether that has any effect on my gop update issue.

Nope, that didn't help. :(

there's some strange kludge in the glist_doreload routine

Hmm, I see now that this was added in the same PR. I thought it was something we inherited from Pd-l2ork.

One thing you could do is go through the zip files on this issue and see if they all work without crash/error in your new branch:

Well, the bonker2 example crashes in my new branch.

So now I have the choice between a crash bug and gop updates not working. :((

Ok, let me see whether I can boil down the patch that's giving me trouble with the gop update, so that you can try your teeth on it. I'm running out of ideas.

Leaving aside the (unavoidable) changes for the nw.js port, there's some strange kludge in the glist_doreload routine where it calls canvas_create_editor(gl) to work around cases where canvas_vis doesn't quite do the right thing.

Yeah that was an awful kludge for an insidious bug.

I'm sure there's a way to clean all that up, but there's just so many side-effects in that part of the code I didn't have time to do it.

Anyway, you're right, there's no bug here, that call to gobj_shouldvis really didn't make any sense there. I just didn't understand what the real purpose of glist_getcanvas is. Also, the bug is really an omission in the JS code. So feel free to remove my ill-informed remarks. :)

I just didn't understand what the real purpose of glist_getcanvas is.

I'm pretty sure IOhannes didn't either, because he used it in do_create_abstraction of s_loader.c to set the search path. Setting the search path based on which canvas is used to draw the canvas doesn't make any sense. In Purr Data it is changed to canvas_getrootfor.