[rcore][GLFW] Fix GetClipboardImage() creating new connection under X11#5871
Conversation
|
|
||
| // Request the data: "Convert whatever is in CLIPBOARD to image/png and put it in RAYLIB_CLIPBOARD_MANAGER" | ||
| XConvertSelection(dpy, clipboard, targetType, property, win, CurrentTime); | ||
| XConvertSelection(platform.cbDisplay, platform.cbAtom, platform.cbTargetAtom, platform.cbPropertyAtom, platform.cbWindow, CurrentTime); |
There was a problem hiding this comment.
ColleagueRiley's reference also calls XSync after this, do we need to add it here?
There was a problem hiding this comment.
It's not really required because you later wait for the SelectNotify event, but it may still be good to have.
The XSync() function flushes the output buffer and then waits until all requests have been received and processed by the X server.
|
I just looked into commit history, it seems RGFW is also affected? Do I port these to RGFW platform as well? |
|
There should generally be possible to use the original GLFW X Display and X Window rather than creating your own internal display. But, in my opinion, hacky backend fixes like these don't really belong in Raylib, as they make Raylib harder to maintain. This is exactly why the RGFW backend exists in Raylib, because the RGFW is more actively worked on and the implementation is easier to understand and modify. Features can be added directly into RGFW rather than being forced into each backend. Of course, that means that GLFW users will have to wait for GLFW to get updated, but I think that's for the best since for Raylib shouldn't have to maintain these messy hacks throughout each backend. The I have also seen plenty of these sorts of hacks being the result of AI generated code. Ray isn't as familiar with each low-level backend as someone like me, or a GLFW maintainer and I don't believe Ray directly tests for Linux or OSX. So dubious low level backend hacks will create hard to find bugs directly in Raylib, rather than being caught filtered out by RGFW/GLFW. |
Hello, Riley, thanks for your insight. At some point, I also thought of using raylib's main connection for this but then one would have to deal with event loop management. What if a Key or a Mouse event fires in the middle of this process? I suppose that's why it was made like that in the first place (even though I don't like it personally). I agree that this is a hacky solution, having platform-specific code in a platform-agnostic backend is not a good idea. I suppose for Windows it worked because it's a few lines of code but for Linux, the situation is different (considering, there's also Wayland support wanted). If RGFW supports this out of the box, will be awesome but I'm also concerned about the GLFW backend. My main issue with this whole thing is that because of this, projects using static raylib have to be explicitly linked to X11. The way it was before, GLFW would dynamically load necessary X11 functions and you would simply link raylib without additional flags (maybe aside from -lm), so the very first claim:
Would be true. Not sure about AI generated but this does feel like a PoC code pushed into the upstream. So now that raylib does require X11 linkage, I want the implementation to be somewhat usable in real projects. |
|
I think your first point is somewhat true. Though, I'm pretty sure you are able to resolve this by peeking the event, seeing if it's the one you want and if not, put it back in the queue. Yeah, I don't think it's even a good idea for winapi code due to maintainability issues. But you bring up some other points that I didn't mention. Generally for RGFW I have two methods for handling wayland/x11, either the user has to choose one at compile time, making it incompatible with the other, or you can actually compile for both backends, and then choose the preferred one on runtime. It will also fallback to X11 if wayland fails. While that does lead to a larger binary, I think that's generally worth it as it will ensure both platforms are supported by default. But Raylib can handle that however it sees fit. I don't really consider X11 or Wayland to be an external dependency as they're core parts of the Linux platform. Dynamically loading doesn't necessarily fix the problem, as it requires the headers, but RGFW has a way of doing that too via XDL. But I would still consider dynamic loading to be a dependency. I have seen AI code in other PRs too, but specially with the original changes for this feature @CrackedPixel and I saw some tell tale signs of AI. For example, how there were a ton of comments all over the code that were later removed. |
GetClipboardImage() creating new connection under X11
|
@ColleagueRiley, I found a way to use the main connection to handle the clipboard event without disrupting the event queue, I want your opinion. Tested with clipboard image example, theoretically it should work. |
|
That looks good, since it also removed the SelectionNotify event form the queue. Although there is no reason to use while-do rather than just doing |
|
Done. I need more reviews for this one, then I may port this to RGFW backend until it gets native support for clipboard image processing. |
|
It would be fairly easy for me to add it to RGFW, the only problem is that I would want to implement each backend as well and I'm not sure how long that would take. Although, I already have a plan for how the API would look. |
|
@steampuker thank you very much for the improvement and thanks @ColleagueRiley for the review |
|
@raysan5, please note that RGFW still has the suboptimal code, I could port this to the RGFW backend (at least for now) |
As of now, GetClipboardImage creates a new connection and a ghost window to retrieve image data and then unloads it all. This adds unnecessary overhead for this function caused by establishing and terminating connection.
This PR
stores this clipboard connection and other related data in the global platform struct but is also lazy loaded (so a first call would have the overhead and subsequent ones would not). The clipboard handling data is unloaded upon ClosePlatform.Update 18/05/26: Now this PR makes the use of the main raylib connection without creating new ones at all.
Tested with ubench:
Without the fix, the result is:
With the fix: