-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[#5455] Fix for: Fix window width calculation by adding wOffset #5480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@Crisspl thanks for the review! |
|
This to me looks like a very dubious fix and should've had actual review and testing before it was pulled. |
|
i was looking at newest rgfw repo source code when preparing this. Should be fine IMHO (also im running this almost daily and havent encountered any problems so far). But ofc actual review would be appreciated |
|
'Should be fine' and 'haven't encountered any issues' doesn't really kinder any confidence. After doing a little bit of research I found it would be better to replace wOffset/hOffset with AdjustWindowRect a function that uses the proper native Windows method for calculating the window size. Also, this should've been done upstream in RGFW. |
|
@ColleagueRiley I'm afraid I can't maintain this raylib backend myself, already too many backends, are you interested in maintaining it? |
|
tbf, i think RGFW backend should work the same way as SDL backend (and maybe glfw if im not mistaken?). That is: using backend lib's api, not embedding it into raylib in modified form. If it's not possible, then it probably just shouldnt exist, or points out that RGFW api needs more flexibility. |
|
@Crisspl Yes, I agree, external libraries should not be modified in raylib side but in its own repos. Latest RGFW introduces a big redesign and requires a full review of the raylib backend But if nobody can take the ownership of it, I think it'd would be better to remove it. I'll also ask raylib community about it. |
|
@Crisspl Raylib also includes an internal copy of GLFW https://github.com/raysan5/raylib/tree/69861838583a6c5c4603a0117b7133b2af528030/src/external/glfw I'm not really sure what you are talking about. Raylib was not using a modified version of RGFW, most of it is exactly how it was in the original repo until this PR. It is however using an outdated version of RGFW. Custom fixes wouldn't be needed if the version was updated and kept up to date. RGFW in general way pretty early in development when it was implemented into Raylib, there were very few users and very few tested implementations and plenty of features were rushed and added into Raylib to be compatible with GLFW. RGFW 2.0 will be the most mature release of RGFW, with a pretty solid API that has been testing across far more projects and use cases. Using arbitrary version numbering for RGFW is probably one of my biggest regrets as it has created a lot of confusion. From 2.0 and onward RGFW will be using semver. But all of the versions older than 2.0 really shouldn't have been considered full releases of the API. |
|
@raysan5 yes, but imo you should have reviewed the PR itself. This seems to be a pretty common problem with Raylib where you do not actually do any proper code review or testing. You can't really assume the code itself works and is good just because it claims to be, especially in the age of AI slop. If you don't have the time to do such a review maybe it would be worth having a system to have a trusted contributor/moderator review changes before they're pulled. I don't know that's just an idea.
Yes, but also the RGFW backend never got a full review in the first place. Many bugs came up later on and many probably still exist in the backend. Simply because I implemented things incorrectly in the backend because it was mostly developed by me in my spare time and I have never used Raylib. One example of this would be that Raylib has a mode flag 'borderless' and also a flag called 'undecorated'. RGFW uses the term 'borderless' to 'mean window with no border/decor, only pixels', Raylib uses it to mean 'borderless fullscreen'. It was my fault for getting this wrong and it was fixed later on by a Raylib user, however this is something that should have been caught before the RGFW PR was pulled. |
|
@ColleagueRiley I do my best to review all PRs, I can't do a detailed test of every single change in the library unless the contributors provide it. I also trust some contributors, specially if contributed to the project in the past. If you are ready to maintain that backend that uses your library, I trust you, RGFW is your library, but I won't be testing every added feature in the same way as the backends I implemented and I actively maintain. |
I think it took someone in the discord one review to notice the flaw, since they are familiar with the code base. But you aren't familiar with RGFW either so that's understandable. The comment about RGFW there was only some-what related, to say that the backend itself contains errors because it wasn't developed by someone familiar with BOTH Raylib and RGFW. As far as maintaining the RGFW goes, unfortunately can't justify committing to it. I'm sure you would agree that it would be great for marketing and community for both projects. I can't deny that this backend caused the initial success of RGFW and I am very grateful for that. I would liketo update the Raylib backend when the 2.0 backend comes out, but without any support it's really hard to justify the level of time, focus and effort it would take it would take to do it right. It's already been a struggle to find the time and motivation to develop RGFW, though I am very happy about what I've accomplished in that time, and the only incentive to maintain the Raylib backend is the possibility of exposure. ..Hopefully when I'm not suck working a retail job I will have more time and motivation to contribute more to community projects. |
|
@ColleagueRiley thanks for your answer, adding @CrackedPixel as a potential maintainer of raylib RGFW backend for the future. |
Well, sorry for confusion then. I was pretty sure that it does, but now I don't know why. Thanks for clarification...
Regarding SDL, i was referring to this: raylib/cmake/LibraryConfigurations.cmake Line 117 in 4c71625
It looks like this cmake code allows SDL to be provided externally |
Related to #5455
Previous fix ( #5457 ) didnt compile.
Confirmed that it builds and works fine