search

lv2 / pugl

A minimal portable API for embeddable GUIs
https://gitlab.com/lv2/pugl/
ISC License
174 stars 34 forks source link

Make X11GlEnter fail if the window is not realized #77

Closed jpcima closed 2 years ago

jpcima commented 2 years ago

When a GL view is created, and then destroyed before its realization, the event PUGL_DESTROY dispatches and attempts to enter the uninitialized GL context, producing a crash.

drobilla commented 2 years ago

Sigh, OpenGL context is why we can't have nice things :)

I'll check it out, thanks.

jpcima commented 2 years ago

If you grab this repo with the branch pugl-pr-77. It builds the plugin gain.vst3 which hits the problem as it starts. https://github.com/jpcima/claptrap/

jpcima commented 2 years ago

So it's not just x11, all the platforms have the problem. Perhaps the better idea to prevent dispatching PUGL_DESTROY in such cases?

drobilla commented 2 years ago

Yeah, I think your fix is fine (and maybe resilience here is good regardless), but it also seems like a symptom of a deeper problem (the event semantics aren't super solid or tested yet, I've been working on vaguely this sort of thing lately over on blank-flush).

Not certain off the top of my head what PUGL_CREATE and PUGL_DESTROY should mean, but knee-jerk, I agree it seems like these shouldn't be dispatched on an unrealized view in the first place...

In any case, I pushed a fixup that adds a unit test since this is an easy one to catch.

drobilla commented 2 years ago

maybe resilience here is good regardless

... on second thought, maybe not. Backends are ideally simple, the core shouldn't be making them handle weird situations if it's avoidable. I'm not sure how the details will work out though.

drobilla commented 2 years ago

Okay, I think it would be better to have the core avoid this, but to do that you need some state that isn't there right now. I happen to have already added a state enum for vaguely similar purposes in blank-flush which should be usable for this as well.

So, I think I'll merge your fix as-is to get the bug fixed on master ASAP, and consider cleaning up CREATE/DESTROY and removing these checks along with the other event/state/etc work.

drobilla commented 2 years ago

Merged as d6ad1b79, thanks.