Open sjaehn opened 1 year ago
I dumped view->lastConfigure.x
and view->lastConfigure.y
during running B.Amp in puglViewSize()
https://github.com/lv2/pugl/blob/609aed744bf205c992a10dab79109e3d6df1991d/src/x11_cairo.c#L23-L39 and both were 0 (as expected) in JALV but 100 in Carla (which is (estimated) the offset there).
If I do the same with B.Angr and force a full window redraw, I get -350, -130 which also looks like the negative offset in the screenshot mentioned above. I think these values are linked to the cause of the problem.
Edit: Typo pugl -> jalv
NOT observed in old pugl builds (Jan 2021) I used before - Therefore I think it's pugl-related.
You mean currently, with the same current version of the same hosts, the old pugl code works properly? Would those offsets happen to correspond to partial redisplays you're doing? Does the issue still exist if you only ever do full redisplays of the entire view?
and both were 0 (as expected) in pugl but 100 in Carla (which is (estimated) the offset there)
I don't understand this. How are you printing offsets within the pugl implementation "in Carla"?
I don't understand this. How are you printing offsets within the pugl implementation "in Carla"?
A quick and dirty fprintf(stderr, "%i %i\n", view->lastConfigure.x, view->lastConfigure.y);
hacked into the pugl code in line 29 of the code snipplet above. Then compiled B.Amp or B.Angr against this pugl version. Loaded into Cairo. Output in the Cairo log.
So... also in pugl. How is it zero in pugl and also 100 in pugl? Is the first "pugl" supposed to be another host or something? "the Cairo log"? What is happening? :D
Sorry typo, my fault....
Post 2 corrected. I typed "pugl" and meant "jalv".
I see. Logging the actual configure events that come in would narrow down if there's some weird bug that makes those numbers come from some botched calculation in pugl itself.
It sounds like the host is setting an offset and Pugl is just doing what it's told, though.
Likely relevant variable: jalv and ardour embed the UI. I don't think the others do?
Back to your questions in post 3:
You mean currently, with the same current version of the same hosts, the old pugl code works properly?
My plugins compiled against the old pugl (Jan 2021) work fine with Carla and the others. All my plugins compiled against the new pugl (yet) produce these offsets.
Would those offsets happen to correspond to partial redisplays you're doing? Does the issue still exist if you only ever do full redisplays of the entire view?
I'm going to try puglPostRedisplay()
(without "Rect") in BWidgets::Window::onExposeRequest()
(this is the only call of post redisplay for the whole TK) with B.Amp when I'm back on my PC. I'll see...
Weird. I need better tests that emulate whatever situation this is.
Anyway, let me know. FWIW the point of the pendingConfigure
stuff is that it eliminated poorly-maintained redundant copies of the same data, so once the kinks are ironed out, there will definitely be less bugs.
Tested puglPostRedisplay()
(without "Rect"): No improvement.
Some interesting traces using B.Amp (which has got a 100x100 display), and I think I've got a possible cause:
getInitialFrame()
https://github.com/lv2/pugl/blob/609aed744bf205c992a10dab79109e3d6df1991d/src/x11.c#L477 returns {-50, -50, 100, 100} in jalv, but {100, 100, 100, 100} in Carla. Both calculated from the third condition ("Get the position/size of the parent as bounds for the new window"). {parentAttrs.x, .y, .width, .height
} were {0, 0, 1, 1} with jalv, and {0, 0, 300, 300} with Carla.
The result is exclusively used in puglRealize()
in https://github.com/lv2/pugl/blob/609aed744bf205c992a10dab79109e3d6df1991d/src/x11.c#L576-L587
If initialFrame.x
and initialFrame.y
are set to 0, then everything works as it should be. In both, jalv and Carla. But I need to do some additional tests with the other hosts and also standalone.
I hate everything about window positioning so much. Anyway.
The initial position is, in a top-level WM sort of context anyway, basically meaningless and gets immediately overwritten if it's ever used at all.
This seems to be a result of attempting to center the window anyway, IIRC in response to a ticket from falktx. I don't quite entirely get it yet (I'll try to repro myself later if I find the time), but I suspect both sides are wrong. Pugl may be calculating something incorrectly here - in particular, in the embedded case, using the parent position is probably wrong because the child's coordinates are relative to that - but at the same time, if hosts want to adjust things after the fact, they need to, well, adjust things after the fact, completely and consistently, which doesn't seem to be happening.
Checks with initialFrame.x and initialFrame.y are set to 0: [✓] Standalone app (B.Widgets widgetgallery) [✓] plugin in jalv (B.Angr and B.Amp) [✓] plugin in carla (B.Angr and B.Amp) [✓] plugin in reaper (B.Angr and B.Amp) [✓] plugin in zrythm (B.Angr and B.Amp) [✓] plugin in ardour (B.Angr and B.Amp) [✓] plugin in qtractor(B.Angr and B.Amp)
Put another way, I think Pugl is doing an incorrect calculation here, but it shouldn't matter. Things should work properly if the initial position is literally random numbers. The host is the window manager here, so it is responsible for managing windows.
What's the 300? Is that the actual size of the UI? If so, how did Carla get that information (since the window doesn't exist yet to get it from)? If not (i.e., 300 is just some made-up thing later adjusted), and pugl is correctly centering in the window that it's initially given to embed in, then this is a host bug.
The ticket about centering on the parent is around somewhere but I'm too lazy to look it up. I'd be fine just making it always 0 if that fixes things, really, but in the event of being embedded in a larger window than you need, centering by default is nice. In the LV2 case, though, hosts are inherently responsible for that (if they resize anyway) so it shouldn't really matter.
In regards to Carla at least, it never sets x or y position offset of the window because it never has a need for it. For the moment Carla always creates a simple window to give to LV2 plugin UIs, it does not have any extra top-bar, title-bar or alike.
my suspicion is that the "broken" hosts leave the (actually broken) pugl offset alone which leads to the issue. ardour manually positions the embed x11 window, forcing it in place, so issue is bypassed there.
If Carla is initially giving this 100x100 UI a window sized 300x300, then the calculation that determines a position of 100,100 seems correct to me.
Carla seems to initially provide 300x300 to everything, whereas jalv only 1x1.
It is using a 300x300 initial window size yes https://github.com/falkTX/Carla/blob/main/source/utils/CarlaPluginUI.cpp#L97 But then as I mentioned it never touches the offset again, I believe
Right. So the plugin UI is centered correctly, then Carla resizes the window, and does not change the position. This is a bug. Nobody said the window position had to be 0,0. Quite the opposite, this feature was specifically requested.
if you change jalv to give a 100x100 initial size, would it then have an offset of -50?
I don't think jalv ultimately gives a damn, but I don't know off the top of my head since there's both gtk and suil shit going on. The negative is because the parent window size is silly.
Quite the opposite, this feature was specifically requested
I remember requesting this yes, but only for root windows or those with a transient parent. Having embed windows at 0,0 seems to be the common practice and expectation, seeing how other hosts do it right now
Fair enough, but, well, no. I will literally get complaints about it being not centered, which I have in the past several times, beyond that ticket.
If you want to resize the parent window, you are also responsible for positioning the child as you like. That's the only way that's possible and/or makes any sense. Centering is a reasonable initial default but Pugl can't do anything about this after that.
if you change jalv to give a 100x100 initial size, would it then have an offset of -50?
Yes, as mentioned above. But this doesn't produce clipping in jalv. No difference if I take the -50 or if I hack in 0, in jalv.
I remember requesting this yes, but only for root windows or those with a transient parent. Having embed windows at 0,0 seems to be the common practice and expectation, seeing how other hosts do it right now
Makes sense
If y'all want to propose an LV2 rule for it, go nuts, I'm not opposed to it. In current reality, this is a Carla bug.
In the mean time, the plugin UI can work around it by setting an initial position of 0,0 itself. Pugl only does this calculation when nothing is set (same for size). If this is to be the rule, that is the correct thing for an LV2 plugin UI to do regardless.
I think that should be the rule to follow, as afaik it is what all other specs and plugin hosts are expecting. Carla is just one of many making this assumption.
Yes yes, all the developers want everything to be someone else's problem. I know the game well.
... however, I think my argument is better ;) This is a general purpose UI abstraction layer. Centering is a reasonable default thing to do, and will make things look sensible in many scenarios. Those scenarios, like the parent being "too big" and nothing bothering to do any later adjustments actually happen in real life.
The plugin UI can, and should, explicitly set its position to whatever it wants if that's important (in this case 0,0
, apparently). Carla should also not make assumptions about the window position. In other words, neither should be making these assumptions. Anyone and anything that cares about window positions needs to either check or explicitly set them, always, or everything will continue to be janky forever.
(This is me being deliberately stubborn in pursuit of the greater goal of having the LV2 ecosystem as a whole be less fucked up and sketchy. That said, I'm also pretty tired of all this window positioning shit. I've done quite a bit of work to rework the APIs based on feedback from all y'all, provide clean ways of setting (or not) position and size, both initially and later on if necessary, separating position from size, and so on, so... use it. Then you never have to care about whatever the hell defaults pugl calculates ever again)
sure but pugl is used for more than just LV2, and we need to consider those cases in real life scenarios. while I can fix up Carla, we cant do much about Ableton, Bitwig, Reaper etc etc..
So far this behaviour causes breakage on Reaper, and we only have that point of information due to @sjaehn mainly targetting LV2. I can imagine that if I went to try other formats things would look incorrectly offset in quite a few additional places.
it is tricky for sure... on one side it is good to force good behaviour out of other tools, on the other what users will assume is broken is going to be the plugin or its framework if pretty much everything else works "correctly".
it is tricky for sure... on one side it is good to force good behaviour out of other tools, on the other what users will assume is broken is going to be the plugin or its framework if pretty much everything else works "correctly".
Indeed. Which is why I might change it... later. All plugins specs are a complete nightmare of a mess for exactly this reason, hosts and plugins pointing fingers at each other for everything. LV2 is especially bad in some ways due to its anarchic nature, but they're all bad. We need to do better.
The way I think we do better in this case is described above, which is exactly what I think the spec should say, if "we" get around to it. Such specifications should have resilience built-in wherever it's not too onerous to do so. Which means, in spec-ese:
Plugin UIs SHOULD initially position themselves at the origin of the provided parent window.
Hosts MUST completely adjust the child window (position, and size if necessary/permitted) whenever they change the size of the parent.
The first is a SHOULD because this is inherently unreliable, and initial window positions are, usually in general, meaningless on X11 in particular. Hosts shouldn't depend on it because that only results in flakiness, i.e., this is the resilience built-in to the spec. The latter is a MUST because it's the only thing that makes sense in the grand scheme of things, not doing it only results in bugs, and it is the other side of said resilience. This means that if the plugin fucks up - which they will - the UI will still be drawn at the correct position (otherwise the host is "officially" broken). It also means that if the host fucks up - which they will - things will still probably display properly. This is the correct distribution of complexity, I think, following the general LV2 principle of hosts ideally being responsible for things when there's a choice (because there's far fewer of them, plugins should be as simple as possible to make, and so on). In this case, both are supposed to do the thing that makes things work properly, the host is just required to.
(There's a few other necessary rules, like "Plugin UIs MUST NOT reposition themselves after initially creating their window", but the above two are the important ones here)
As for pugl's defaults, most LV2 plugin and host developers can, just have, and will continue to want me to make 0,0 the default, obviously. Others can, have, and will continue to want me to make it centering (and they have a pretty good objective argument, since it's not even possible to do that portably outside of pugl itself). So I choose neither: it shouldn't matter, so make it not matter. I might change it later, but I'm pretty confident the above is the right way as far as LV2 is concerned.
Concretely, Carla should use XMoveResizeWindow
instead here. If it wants the window at 0,0 at some particular size within another window, then it needs to do just that. Note that 0,0 is not the only sensible position, and other hosts may do otherwise.
However, all that said, I'm not saying my code here is faultless! Replacing my LV2 hat with my Pugl hat; actually, the API is a bit lacking here. The other side of what should concretely happen here is:
However... the API is a bit confusing and/or under-documented for this. Usually, you do that stuff with hints, PUGL_DEFAULT_SIZE
being the hint for, well, the default size. There's no PUGL_DEFAULT_POSITION
, though, because the API is for size hints, which are unsigned (positions can really be negative). Adding a whole mechanism for "position hints" when I can't think of any more than this one (?) seemed silly. Calling puglSetPosition()
before realizing will do this , though (puglSetSize()
also works before realizing and is the same as setting the hint in that case). It works, but puglSetPositionHint(view, PUGL_DEFAULT_POSITION, 0, 0)
would be more consistent and clear about what it's doing.
I'll think about it. Getting the hints API right so it can be extensible to roughly everything is one of the most important things about finally sticking a big 1.0.0
on pugl. Can anyone think of any other use cases for "position hints"? Nothing else comes to mind, although they can be dynamic, so puglSetPositionHint(view, PUGL_CURRENT_POSITION, 123, 456)
could be a thing. That feels like taking the API generalization (maybe even deprecating puglSetPosition) a little too far for no particularly good reason, though. I'd say mouse position, but manipulating that is generally regarded as evil, and I do my best to refuse any such addition to pugl (even the position stuff was a real struggle, this absolutely shouldn't be pugl's problem).
X11 does have a concept of gravity, which solves exactly this problem, but that's a whole different thing, not in terms of positions, and I'm not interested in anything other than center and origin (north west). It would be a trivial hint to add, though, which avoids the 2-dimensional problem, but the ability to set an arbitrary position is necessary anyway so that seems like completely pointless complexity.
I suppose I'll keep live-blogging my API design thoughts here since I've started:
I was going to say it might be less weird without "hint" in the name, but come to think of it, setting your own position is entirely unsupported on Wayland anyway, and most hints are also not supported in many cases. So in that sense, "hint", meaning "a suggestion that might be used, but provides no guarantees whatsoever" is actually totally consistent with the window position, at any time. It jams pretty much all of this distasteful window management fluff into the hints mechanism, which also makes it super easy to add more as things come up. I quite like it, actually.
puglSetPositionHint(view, PUGL_DEFAULT_POSITION, 0, 0);
puglSetSizeHint(view, PUGL_DEFAULT_SIZE, 640, 480)
then, maybe, later in execution (in response to a configuration change, for example):
puglSetPositionHint(view, PUGL_CURRENT_POSITION, newX, newY);
puglSetSizeHint(view, PUGL_CURRENT_SIZE, 800, 600)
Applications have the choice whether the current position/size should also be used as the new default if the view is re-created, or not.
Seems pretty nice to me. There's a reason nearly every API even remotely like this multiplexes on a "key" like that for a bunch of things. So far, it seems things turn out pretty nicely when you take that idea and drive it as hard as possible, including more data types, position, sizes, and using the same design dynamically as well. It's like the UNIX philosophy of windowing APIs, and the more I think about it, the more I'm convinced that the best way here is to go all plan9 about it and really do it...
Concretely, and relevant for LV2 and the fact that this library is going to hit distributions soon, it's also super resilient across versions. The implementation can easily be fine with being linked against plugins targeting newer versions (if hints are the only difference) since it's just a dynamic check of an integer at the end of the day.
ICCCM has both a "user-specified" position, and a "program-specified" position.
I wonder how widespread proper support for that is, it's a pretty good idea.
Here's the best I can come up with:
https://github.com/lv2/pugl/blob/ba368e40cc6c77775a0f4a6a363247124cf14819/include/pugl/pugl.h#L951 (and the corresponding new size hints below it)
This makes the API distinguish between default, program-derived, and user-derived positions and sizes, so there's clear and distinct cases for all the positioney things that can happen. Pugl is, itself, essentially setting the "program-derived" PUGL_CALCULATED_POSITION
(although things aren't literally implemented that way at the moment on that branch). Things like LSP moving itself would also be PUGL_CALCULATED_POSITION
.
It also explicitly decrees that if the user code sets no position hint whatsoever, it's some arbitrary thing (it would be possible to preserve either the origin or centering behaviour of unmodified earlier code by also adding a gravity hint, but I don't particularly care about that right now).
Also:
So far this behaviour causes breakage on Reaper, and we only have that point of information due to @sjaehn mainly targetting LV2. I can imagine that if I went to try other formats things would look incorrectly offset in quite a few additional places.
To be clear, again, (and again, and again) this is an unreleased git repository. I am literally working on the API itself, which is very obvious to anyone tracking it. Using pugl - an unreleased and explicitly unstable project in development - from git and updating it isn't even guaranteed to compile, let alone work in exactly the same way. Where feasible I maintain the deprecated API macros for a while as a courtesy, but that's all, and I'll be deleting all of that shortly enough, too.
This issue has helpfully highlighted an issue with the API, or at least its documentation, and the fix is this:
Pugl code must not make any assumptions about the position unless it has explicitly set one, or has received a notification about it through a configure event.
That is the API, whatever the default may be this week in whatever scenario. I'll consider if it's possible to make not setting one for embedded views a hard error, but in any case, the fix is the same.
Thanks, some good ideas from you.
- Plugin UIs SHOULD initially position themselves at the origin of the provided parent window.
- Hosts MUST completely adjust the child window (position, and size if necessary/permitted) whenever they change the size of the parent.
Agreed. This is what I expect, too.
(There's a few other necessary rules, like "Plugin UIs MUST NOT reposition themselves after initially creating their window", but the above two are the important ones here)
Agreed. Although I know that some plugin authors try (or tried) to directly manipulate it. And caused glitches.
B.Angr and friends should set the default position to 0,0 before realizing the view.
Agreed, this is what I want and what I need (as a consequence of the point mentioned above).
puglSetPositionHint(view, PUGL_DEFAULT_POSITION, 0, 0);
puglSetSizeHint(view, PUGL_DEFAULT_SIZE, 640, 480);
This is in case of / at least for plugin windows.
puglSetPositionHint(view, PUGL_CURRENT_POSITION, newX, newY);
puglSetSizeHint(view, PUGL_CURRENT_SIZE, 800, 600);
In the case of plugins, the second one is (also) intended to request a resize to the host, isn't it? This could be an alternative to the deprecated:resize mechanisms in LV2_UI.
The first one might be conflicting with the first rule mentioned above, in the case of plugins. Or? Anyway, I think this is needed as well.
To be clear, again, (and again, and again) this is an unreleased git repository ...
This is fully clear, and I know.
Pugl code must not make any assumptions about the position unless it has explicitly set one, or has received a notification about it through a configure event.
Agreed.
In the case of plugins, the second one is (also) intended to request a resize to the host, isn't it? This could be an alternative to the deprecated:resize mechanisms in LV2_UI.
Ideally, yeah, although I haven't figured out how to catch it properly in all cases in suil yet (through all the Gtk layers, without completely rewriting the plug/socket stuff anyway). I would really, really like to figure that out, but so far, I've failed...
Pugl API-wise, ideally I think puglSetPositionHint
and puglSetSizeHint
should be the only position and size setters. Now that I've really dug into this and thought about all the resize/position cases/problems that have come up, I realize it's inherently trouble to have a "set position" and "set size", because there's isn't really just one position or size. We'll be a lot better off in the future if code is forced to declare specifically what "kind" of size it's setting. That said, I really am trying to stop changing the API ASAP so I'm a bit miffed at deprecating such straightforward functions. We'll see.
I'm not sure if three kinds (default, program, and user) is overkill or if just two will do. I need to see how things can be properly glued to the other platforms. Conceptually, the three covers all the cases, I think, but there's no actual point to that in pugl unless it can do something useful with them through the platform APIs it works with internally. That said, it might be useful for larger plugins/programs to be able to, say, set the calculated size, but not nuke a user size (which should usually override it) you might have set earlier somewhere else.
This is fully clear, and I know.
Yeah, I was just saying for the crowd really. I know you know what you've signed up for, and I appreciate coming along for the ride so we can get the best possible layer here we can. I thought size/position API stuff was pretty much done, but I guess not. Almost! :sweat_smile:
So this is going relatively well, but I've arrived at one thing that gives me a bit of pause: how to decide whether to actually apply a new size when hints change. What I currently have is a sort of high-level catch-all: store the previous "hinted" position, update hints, store the new "hinted" position. If the old hinted position was the actual position of the window (from the last configure), then update the position to the new position. That "if" is the slightly sketchy part: it would technically be possible to have the window moved back to a position that matches the hinted one, but for some other reason (WM or host whatever), in which case pugl would move the window when it maybe shouldn't. Same goes for size.
Solving this requires either tracking the position and size to see if it ever moves and is no longer "pugl directed" (complex, probably even flakier because there's new sticky state around), or... coming up with different rules that I can't imagine.
That said, I think it's probably fine, because you really shouldn't be setting hints all over the place while visible unless you maybe want to move the window.
Does the mysterious size/position that comes from configure events count as one of these kinds of "hint"? Does it matter? There's not really any information about that except the numbers. I don't think it matters much, but since I'm dumping time into trying to Do This Right, it irks me that there's any potential whatsoever for unpredictable behaviour, however marginal.
WIP branch here: https://github.com/lv2/pugl/tree/position-hints
This is fast. I'll give it a test with B.Amp and the host mentioned above this weekend.
No rush. It should be pretty much the same for you anyway, I'm just trying to make the API more coherent.
Ah, yes. I see. initialFrame
in puglRealize
would still be the same. And therefore still the offset. I'll have to wait.
To be clear just doing puglSetPosition(view, 0, 0)
before realizing should fix this issue for you now, and is basically the same as what will come except with some API rearranging to hopefully make things more obvious.
Talking about the position-hints branch?
Nope. It doesn't work this way. It only works if called AFTER puglRealize()
. And then only with values != 0 (like 1,1). Otherwise the condition
https://github.com/lv2/pugl/blob/82fc5af57b451c3c0c6cb1b8c6e97e2ad4329c5f/src/x11.c#L2009-L2011
would prevent calling puglSetWindowPosition()
and thus XMoveWindow()
. I think this was not your intention.
... calling puglSetPosition
(or now better puglSetPositionHint()
) prior puglRealize()
can't work yet. puglRealize()
still calls XCreateWindow
with the same initialFrame
calculated from the third condition (fallback) in getInitialFrame()
. Where frame centering is done which caused the clipping problem.
... a possible solution would be the substitution of https://github.com/lv2/pugl/blob/82fc5af57b451c3c0c6cb1b8c6e97e2ad4329c5f/src/x11.c#L541-L548
by someting like
const PuglPoint hpos = puglHintedPosition(view);
const PuglRect frame = {hpos.x, hpos.y, defaultWidth, defaultHeight};
return frame;
Then it works prior puglRealize()
.
... but all positionHints[]
are zero-initialized and thus (0, 0). And (0, 0) is puglIsValidPosition()
true. Thus, the first check in puglHintedPosition()
will return the zero-initialized positionHints[PUGL_USER_POSITION]
and thus shadows puglSetPosition()
(which sets PUGL_CALCULATED_POSITION
) and puglSetPosition(view, PUGL_DEFAULT_POSITION, ...)
Maybe fill all positionHints[]
with something < INT16_MIN
, except for PUGL_DEFAULT_POSITION
?
... calling puglSetPosition (or now better puglSetPositionHint()) prior puglRealize() can't work yet. puglRealize() still calls XCreateWindow with the same initialFrame calculated from the third condition (fallback) in getInitialFrame(). Where frame centering is done which caused the clipping problem.
You're talking about the main branch or the position-hint branch? Anyway, Your "use the default position" condition is only called on two byte int systems, as x
and y
are defined:
https://github.com/lv2/pugl/blob/bd4f79646f623e929e6aa22bea028952b515aeef/src/common.c#L142-L145
And even than I don't think you want to call XCreateWindow()
in puglRealize()
with INT_MIN
coordinates. Or?
I observed that the visual content written to the pugl-provided Cairo context is displayed with an offset in Carla, QTractor, Zrythm, and Reaper. And the content can't be moved (at least I don't see a way). So all the time parts of the plugin content are clipped. The unwanted offset is sometimes positive, sometimes negative. Here an example with the current master of B.Angr (build with the current pugl) in Carla:
As there's an offset between the host-provided window area and the plugin-used area, the area not used by the plugin will become full of glitches with time (here: bottom and right). Especially on window resize. Likely as the plugin can't write on this part (to clean it).
Additional information: