Merge lp:~qcumber-some/widelands/bug704637 into lp:widelands

[Jens Beyer]

Just as hjd proposed in the bug, simply try to fall back to default settings if the other two tries fail.

Interesting "feature" is, that the setting from .config file is still visible in the config screen, while the window is in tiny 640x480. I don't think this is a bug, but you may comment on that.

I don't know if we should tell the user that something's wrong, this could be quite difficult this early in the game. On the other hand, if I am faced by this tiny window, I am definitely looking for the graphics configuration ingame to fix it, so I will see my mistake.

[SirVer (sirver)]

Thanks for tackling this!

Move the second check (!sdlsurface) into the first - otherwise you check twice for the same condition. I suggest merging the log's into one. And I suggest making the minimum graphics explicit via some consts in constants.h or widelands.h.

review: Needs Fixing

[Jens Beyer (qcumber-some)]

Updated as suggested.

review: Needs Resubmitting

[SirVer (sirver)]

Cool. One more thing I noticed: when disabling opengl, g_opengl (a global variable) must also be set to false. Can you make sure this is the case here? I think this is then ready for merging - feel free to do it yourself or ping me so I can do it. Thanks for fixing this issue!

And secondly - this will still seem strange to users. They choose one mode but they get a completely different one without noticfication (because most will not read the console output). I have no idea how to solve this properly though - some kind of state must be passed back and conserved till we could display a message box. Well, we can find a solution in another patch for this.

review: Approve

[Jens Beyer (qcumber-some)]

I didn't do anything to OpenGL.

If I understand the code correctly, the OpenGL stuff is before, and when we arrive at line 31, in the case of !sdl_surface, opengl is already false anyway, and flags is set &~SDL_OPENGL.

So the handling of the OpenGL stuff is completely untouched by me.

Maybe someone else can check on this, too. If noone objects, I'll merge this tonight.

[SirVer (sirver)]

I just checked the code and you are right, this does not need any changes to g_opengl. Thanks for caring for this bug!

=== modified file 'src/constants.h'
--- src/constants.h	2013-02-11 19:51:33 +0000
+++ src/constants.h	2013-02-28 15:58:25 +0000
@@ -41,6 +41,10 @@
 #define XRES 800 ///< Fullscreen Menu Width
 #define YRES 600 ///< Fullscreen Menu Height
 
+#define FALLBACK_GRAPHICS_WIDTH 640
+#define FALLBACK_GRAPHICS_HEIGHT 480
+#define FALLBACK_GRAPHICS_DEPTH 16
+
 /// \name Fonts
 /// Font constants, defined including size
 //@{
 
=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc	2013-02-16 08:02:26 +0000
+++ src/graphic/graphic.cc	2013-02-28 15:58:25 +0000
@@ -119,16 +119,24 @@
 	// If we tried opengl and it was not successful try without opengl
 	if (!sdlsurface and opengl)
 	{
-		log("Graphics: Could not set videomode: %s\n", SDL_GetError());
-		log("Graphics: Trying without opengl\n");
+		log("Graphics: Could not set videomode: %s, trying without opengl\n", SDL_GetError());
 		flags &= ~SDL_OPENGL;
 		sdlsurface = SDL_SetVideoMode(w, h, bpp, flags);
 	}
 #endif
 
 	if (!sdlsurface)
-		throw wexception
-			("Graphics: could not set video mode: %s", SDL_GetError());
+	{
+		log
+			("Graphics: Could not set videomode: %s, trying minimum graphics settings\n",
+			 SDL_GetError());
+		flags &= ~SDL_FULLSCREEN;
+		sdlsurface = SDL_SetVideoMode
+			(FALLBACK_GRAPHICS_WIDTH, FALLBACK_GRAPHICS_HEIGHT, FALLBACK_GRAPHICS_DEPTH, flags);
+		if (!sdlsurface)
+			throw wexception
+				("Graphics: could not set video mode: %s", SDL_GetError());
+	}
 
 	// setting the videomode was successful. Print some information now
 	log("Graphics: Setting video mode was successful\n");