-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: fix game screen goes black bug #4980
Conversation
fix: fix game screen goes black bug see the issue for details fixes issue MovingBlocks#4929
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm this fixes the issue! Mostly, anyway :-)
I replicated the issue before merging, and confirmed the same steps after would no longer encounter the problem - after I dismissed the in-world menu and moved ever so slightly to cause the dimensions to refresh and redraw the screen. However, right as you bring up the game from being minimized the background is still black behind the menu.
So this could probably be fixed more thoroughly, but that's a triviality and this fixes the important bits! Maybe we should leave the issue open as a longer priority to see about finding an "earlier" fix that doesn't cause the blackness to occur in the first place. But I have no idea where to find that, and it isn't very important after this fix :-)
Nice work @sid2002CN ! I have left a couple review comments with trivial adjustments needed. A couple things you could do in the future:
- Make sure you look for warnings in your IDE about style warnings and such, if you're introducing new ones (just missing some spaces in this case). You can run a Checkstyle plugin within IntelliJ for more details
- Consider learning about and using "topic branches" rather than your
develop
branch, for :reasons: that a quick Google search will highlight along with how to do it - don't worry about that for this PR though, just made the tweaks in a second commit in yourdevelop
branch and we'll merge them!
Thanks!
@@ -728,6 +728,9 @@ public int height() { | |||
* @param height An integer representing the new height. | |||
*/ | |||
public void setDimensions(int width, int height) { | |||
if(width == 0 && height ==0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor code quality fixes here, explained by the robot :-)
Should this just check for an or rather than and ? In the case we're dealing with both dimensions are zero. But it seems like either being zero would be a case where you'd disregard the instruction and return early.
Additionally, maybe it would be good if we did a logger.warn(....)
to highlight that the set was called with invalid dimensions?
@@ -737,6 +740,9 @@ public void setDimensions(int width, int height) { | |||
* @param other A Dimension to use the width and height from. | |||
*/ | |||
public void setDimensions(Dimensions other) { | |||
if(width == 0 && height ==0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to my other review content in this case shouldn't we be checking other.width
and same for height?
@@ -728,6 +728,9 @@ public int height() { | |||
* @param height An integer representing the new height. | |||
*/ | |||
public void setDimensions(int width, int height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's just obscuring the problem by having the method silently fail.
Preconditions.checkArgument(width > 0);
Preconditions.checkArgument(height > 0);
reproduce the issue and look at the backtrace to see where the problem is occurring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pollend That's true. I am looking for a way to solve this bug more accurately. Do you have any suggestions on where I should start on? That would be so helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the precondition should hit and give you a backtrace and give you information where this is coming from. I don't know off the top of my head why its getting set to 0,0
@Cervator Thanks for your suggestions! This is my first attempt to contribute to an open-source project. I am happy that I have done this in the right way(almost). |
Fix screen goes black bug
Thanks to @Cervator and @pollend's suggestion I found out what actually goes wrong. The width and height of displayDevice are not updated properly as the screen goes back to normal after being minimized. The new solution updates the displayDevice before updating the full-scale if width or height is 0. |
@sid2002CN very nice detective work! However, oddly enough, I still get the black screen after restoring the window 🤔 Not sure if that is a problem on my end, it would be good with another opinion or two. Not sure what's happening there, I tried a few different ways to lose focus / minimize and they all result in the same thing. Black behind the pause menu until I dismiss the menu and do anything to trigger a refresh of the world view. Your fix sounds reasonable, but maybe there are more places to capture? One interesting thing I noticed is that with the game minimized the Windows 10 task bar preview does show the small version of the game window with valid background behind the pause menu. It just doesn't work as I restore the window size - maybe the act of restoring the window is still capturing and storing a 0,0 somewhere? I don't really know what |
Block firePropertyChange if the screen is minimized.
@Cervator Thanks for your feedback. I finally got what actually goes wrong. The updateViewport capturing and storing 0,0 during the screen is minimized. The simple solution will be to block the change when the screen is minimized. After spending some time learning how to use GLFW, I come up with this solution. |
Amazingly well done! I love that you're willing to go this deep into research land to find the proper solution :-) And I can confirm that it works this time! No more blackness behind the pause menu! I did however still find an edge case that provokes the original crash though, so there's one more bit of detective work for you 😁
I dunno if that is capturable similarly or justifies putting the zero check back, now perhaps without the blackness (since it did prevent the crash itself - maybe now it would both not crash and bypass the blackness?). I might suggest having the check there (with preconditions maybe, if that can prevent the crash gentle and be informative) still, even if we resolve the issue. A simple comment explaining briefly why we do the thing would also be helpful So close now! Thank you very much for the effort here :-) |
Only render the screen when it is not minimized.
@Cervator Never thought of testing it in that way. 😁 |
Great to hear about the final fix 👍 Comment would just be a one liner The other change I think documents itself by simply using the well-named boolean. You could read that code without comments as "Ah so if the thing isn't minimized then do this other thing, seems sensible" |
@Cervator Just finished the comment change. |
Well done - I can't find a way to crash it or see black in the background now. And the comment is on point 👍 Apologies for taking a little while to get back to this, but the weekend ended 😁 Approving and merging! Woot! |
Contains
Fixes #4929
How to test
Play the game and then pause the game.
Minimize the game window.
Bring back the game window and resume the game.