fix: fix game screen goes black bug #4980

sid2002CN · 2022-01-11T22:45:08Z

Contains

How to test

Play the game and then pause the game.
Minimize the game window.
Bring back the game window and resume the game.

fix: fix game screen goes black bug see the issue for details fixes issue MovingBlocks#4929

Cervator

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 your develop branch and we'll merge them!

Thanks!

Cervator · 2022-01-13T03:46:31Z

engine/src/main/java/org/terasology/engine/rendering/opengl/FBO.java

@@ -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){


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?

Cervator · 2022-01-13T03:47:12Z

engine/src/main/java/org/terasology/engine/rendering/opengl/FBO.java

@@ -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){


In addition to my other review content in this case shouldn't we be checking other.width and same for height?

pollend · 2022-01-15T03:03:43Z

engine/src/main/java/org/terasology/engine/rendering/opengl/FBO.java

@@ -728,6 +728,9 @@ public int height() {
         * @param height An integer representing the new height.
         */
        public void setDimensions(int width, int height) {


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.

@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.

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

sid2002CN · 2022-01-15T03:54:31Z

@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).
As for >finding an "earlier" fix that doesn't cause the blackness to occur in the first place, I kind of get lost on trying to find where to look for. Does anyone know which class actually set the width and height of the FBO?

Fix screen goes black bug

sid2002CN · 2022-01-16T07:10:45Z

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.

Cervator · 2022-01-16T17:13:47Z

@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 displayDevice.update(); does other than what I can assume. Even before testing I wondered if it might still make sense to have the precondition checks after the attempt to run the update, in case there are cases we don't catch.

Block firePropertyChange if the screen is minimized.

sid2002CN · 2022-01-16T22:24:56Z

@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.

Cervator · 2022-01-16T23:37:58Z

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 😁

Run game, enter world in CoreSampleGameplay
Without opening the pause menu screen hit the Windows button
Click a different window to move the focus
Without otherwise activating the game window click the minimize button on its menu bar
OG crash!

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.

sid2002CN · 2022-01-17T03:56:52Z

@Cervator Never thought of testing it in that way. 😁
It is now fixed with the new commit. It turns out that the game still tries to render the screen after we minimize the game.
I am not sure how to write in-class comments about this properly. Is there a formal format or I can add comments which simply describe what I have changed?

Cervator · 2022-01-17T04:08:46Z

Great to hear about the final fix 👍

Comment would just be a one liner // ... for probably the first change since it includes a property change event and some logic - simply suggest a reason for why we might do / need that., so that it is clear to a reader.

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"

sid2002CN · 2022-01-17T05:21:18Z

@Cervator Just finished the comment change.

Cervator · 2022-01-21T05:16:58Z

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!

Update FBO.java

0a6a68a

fix: fix game screen goes black bug see the issue for details fixes issue MovingBlocks#4929

Cervator requested changes Jan 13, 2022

View reviewed changes

pollend requested changes Jan 15, 2022

View reviewed changes

sid2002CN added 3 commits January 14, 2022 23:20

Update FBO.java

50eed2b

New solution

6367838

Update DisplayResolutionDependentFbo.java

d8edf09

Fix screen goes black bug

sid2002CN added 2 commits January 16, 2022 17:00

New solution

775aae9

Fix screen goes black

2acf8c8

Block firePropertyChange if the screen is minimized.

Solve minimize bug

4b1305b

Only render the screen when it is not minimized.

sid2002CN added 2 commits January 17, 2022 00:15

Minimize bug update

6fbdb21

Update minimize bug

72768a5

Cervator approved these changes Jan 21, 2022

View reviewed changes

Cervator merged commit ba06085 into MovingBlocks:develop Jan 21, 2022

Cervator added this to the 5.3.0 milestone Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix game screen goes black bug #4980

fix: fix game screen goes black bug #4980

sid2002CN commented Jan 11, 2022

Cervator left a comment

Cervator Jan 13, 2022

Cervator Jan 13, 2022

pollend Jan 15, 2022 •

edited

Loading

sid2002CN Jan 15, 2022

pollend Jan 15, 2022

sid2002CN commented Jan 15, 2022

sid2002CN commented Jan 16, 2022

Cervator commented Jan 16, 2022

sid2002CN commented Jan 16, 2022

Cervator commented Jan 16, 2022

sid2002CN commented Jan 17, 2022

Cervator commented Jan 17, 2022

sid2002CN commented Jan 17, 2022

Cervator commented Jan 21, 2022

fix: fix game screen goes black bug #4980

fix: fix game screen goes black bug #4980

Conversation

sid2002CN commented Jan 11, 2022

Contains

How to test

Cervator left a comment

Choose a reason for hiding this comment

Cervator Jan 13, 2022

Choose a reason for hiding this comment

Cervator Jan 13, 2022

Choose a reason for hiding this comment

pollend Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

sid2002CN Jan 15, 2022

Choose a reason for hiding this comment

pollend Jan 15, 2022

Choose a reason for hiding this comment

sid2002CN commented Jan 15, 2022

sid2002CN commented Jan 16, 2022

Cervator commented Jan 16, 2022

sid2002CN commented Jan 16, 2022

Cervator commented Jan 16, 2022

sid2002CN commented Jan 17, 2022

Cervator commented Jan 17, 2022

sid2002CN commented Jan 17, 2022

Cervator commented Jan 21, 2022

pollend Jan 15, 2022 •

edited

Loading