Merge lp:~widelands-dev/widelands/bug1098263 into lp:widelands

Proposed by Jens Beyer on 2013-07-11
Status: Merged
Merged at revision: 6623
Proposed branch: lp:~widelands-dev/widelands/bug1098263
Merge into: lp:widelands
Diff against target: 124 lines (+42/-20) (has conflicts)
3 files modified
src/graphic/graphic.cc (+37/-4)
src/graphic/render/gl_surface_texture.cc (+4/-15)
src/graphic/render/gl_surface_texture.h (+1/-1)
Text conflict in src/graphic/graphic.cc
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug1098263
Reviewer Review Type Date Requested Status
SirVer 2013-07-11 Approve on 2013-07-14
Review via email: mp+174275@code.launchpad.net

Description of the change

Ok, I tried to fix this.

The reason I had to split the init function is that the GL_EXTENSIONS seems to be available only late in the game (it looks like glewInit() does provide it).

Problem now is that it looks like we are leaking a sdlsurface instance there (check the TODO), or not? I am not sure.
And I am not sure if I found all possible paths in the code.

Please review thoroughly ;-)

To post a comment you must log in.
6602. By SirVer on 2013-07-12

Updated catalogues.

6603. By SirVer on 2013-07-12

Fixed a used-after-free.

6604. By SirVer on 2013-07-12

Added an assert() to catch a potential use of this after free.

6605. By SirVer on 2013-07-12

Fixed a crash when there is no soldier in the military site.

SirVer (sirver) wrote :

Thanks for looking into this. I spent too much time on Widelands already today, so I'll postpone this review till next week.

And another comment: could you push your branches to ~widelands-dev in the future? that way others can directly edit your branch which makes talking about nits unnecessary.

Jens Beyer (qcumber-some) wrote :

I moved this branch to ~widelands-dev and will do so in the future :-)

6606. By Nasenbaer on 2013-07-12

Fixed a bunch of default case of switch missing bugs + commented out an unsed macro

6607. By Jens Beyer on 2013-07-12

OpenGL and Gettext are mandatory

6608. By SirVer on 2013-07-13

Scan build script by hjd.

6609. By Nasenbaer on 2013-07-13

Add function to determine the maximum theoretical available NodeCap of a field (without any map object on the field and neighbours) and use that function during seafaring expeditions to check whether a port space in the list of port spaces is valid - further readd a functionality to the map saver to remove all port build spaces from the port space list which will never be usable (this might break scenarios like the atlantean campaign, so we have to take care about port spaces in special scenarios manually, however this seems to be a much more smaller problem) - Fix the atlantean campaign map 1 according tothis change. This commit will unfortunally (as with the last two releases) break the savegames from atl01.wmf.

6610. By Nasenbaer on 2013-07-13

Fixed one default case of switch missing warning

6611. By Nasenbaer on 2013-07-13

Improved the S2 map loader concerning port space loading as well as starting position loading and slightly improved the rest of the port space handling code

6612. By Nasenbaer on 2013-07-13

Update Editor icons for port dock with current ingame version

6613. By Nasenbaer on 2013-07-13

Fix bug found by hjd and scan-build

6614. By Nasenbaer on 2013-07-13

Refix bug found by hjd and scan-build - better do not add new bugs when committing bug fixes ;)

6615. By SirVer on 2013-07-13

Merged changes on the surface cache.

6616. By Nasenbaer on 2013-07-13

Fix wrong description of atlantean hammer and saw and update message catalogues

6617. By Jens Beyer on 2013-07-13

fix build error with is_a macro and const

6618. By SirVer on 2013-07-14

Be more consistent with when to use const, const ref and by-value in draw() methods.

6619. By SirVer on 2013-07-14

Use #pragma pack in a way that more compilers understand it.

SirVer (sirver) wrote :

I reviewed this in r6603. I changed some nits (only formatting really) and changed the TODO to an explanation. I also deleted some dead code that could never be reached - can you look at my changes if they look fine to you and if they are go ahead and merge this in?

review: Approve
6620. By Hans Joachim Desserud on 2013-07-14

Use explicit instead of relative paths for SOURCE and BUILD_DIR

6621. By Nasenbaer on 2013-07-14

Place starting positions randomly in automatic generated maps

6622. By Nasenbaer on 2013-07-14

Fix by hjd: Add assertion to make sure we are not dividing by zero. Fixes bug #1095022

6623. By Jens Beyer on 2013-07-14

Check if OpenGL implementation offers framebuffers

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/graphic/graphic.cc'
2--- src/graphic/graphic.cc 2013-07-13 14:32:49 +0000
3+++ src/graphic/graphic.cc 2013-07-14 11:00:38 +0000
4@@ -140,6 +140,13 @@
5 if (0 != (sdlsurface->flags & SDL_FULLSCREEN))
6 log("Graphics: FULLSCREEN ENABLED\n");
7
8+<<<<<<< TREE
9+=======
10+#ifdef USE_OPENGL
11+ bool use_arb = true;
12+ const char * extensions;
13+
14+>>>>>>> MERGE-SOURCE
15 if (0 != (sdlsurface->flags & SDL_OPENGL)) {
16 // We have successful opened an opengl screen. Print some information
17 // about opengl and set the rendering capabilities.
18@@ -151,9 +158,36 @@
19 throw wexception("glewInit returns %i: Broken OpenGL installation.", err);
20 }
21
22+ extensions = reinterpret_cast<const char *>(glGetString (GL_EXTENSIONS));
23+
24+ if (strstr(extensions, "GL_ARB_framebuffer_object") != 0) {
25+ use_arb = true;
26+ } else if (strstr(extensions, "GL_EXT_framebuffer_object") != 0) {
27+ use_arb = false;
28+ } else {
29+ log
30+ ("Graphics: Neither GL_ARB_framebuffer_object or GL_EXT_framebuffer_object supported! "
31+ "Switching off OpenGL!\n"
32+ );
33+ flags &= ~SDL_OPENGL;
34+ m_fallback_settings_in_effect = true;
35+
36+ // One must never free the screen surface of SDL (using
37+ // SDL_FreeSurface) as it is owned by SDL itself, therefore the next
38+ // call does not leak memory.
39+ sdlsurface = SDL_SetVideoMode
40+ (FALLBACK_GRAPHICS_WIDTH, FALLBACK_GRAPHICS_HEIGHT, FALLBACK_GRAPHICS_DEPTH, flags);
41+ m_fallback_settings_in_effect = true;
42+ if (!sdlsurface)
43+ throw wexception("Graphics: could not set video mode: %s", SDL_GetError());
44+ }
45+ }
46+
47+ // Redoing the check, because fallback settings might mean we no longer use OpenGL.
48+ if (0 != (sdlsurface->flags & SDL_OPENGL)) {
49+ // We now really have a working opengl screen...
50 g_opengl = true;
51
52-
53 GLboolean glBool;
54 glGetBooleanv(GL_DOUBLEBUFFER, &glBool);
55 log
56@@ -184,8 +218,7 @@
57 ("Graphics: OpenGL: Version %d.%d \"%s\"\n",
58 m_caps.gl.major_version, m_caps.gl.minor_version, str);
59
60- const char * extensions = reinterpret_cast<const char *>(glGetString (GL_EXTENSIONS));
61-
62+ // extensions will be valid if we ever succeeded in runnning glewInit.
63 m_caps.gl.tex_power_of_two =
64 (m_caps.gl.major_version < 2) and
65 (strstr(extensions, "GL_ARB_texture_non_power_of_two") == 0);
66@@ -276,7 +309,7 @@
67 SDL_GL_SwapBuffers();
68 glEnable(GL_TEXTURE_2D);
69
70- GLSurfaceTexture::Initialize();
71+ GLSurfaceTexture::Initialize(use_arb);
72
73 }
74
75
76=== modified file 'src/graphic/render/gl_surface_texture.cc'
77--- src/graphic/render/gl_surface_texture.cc 2013-02-10 18:47:18 +0000
78+++ src/graphic/render/gl_surface_texture.cc 2013-07-14 11:00:38 +0000
79@@ -30,28 +30,17 @@
80 /**
81 * Initial global resources needed for fast offscreen rendering.
82 */
83-void GLSurfaceTexture::Initialize() {
84- const char * extensions = reinterpret_cast<const char *>(glGetString (GL_EXTENSIONS));
85+void GLSurfaceTexture::Initialize(bool use_arb) {
86+ use_arb_ = use_arb;
87
88 // Generate the framebuffer for Offscreen rendering.
89- bool fbs = false;
90- if (strstr(extensions, "GL_ARB_framebuffer_object") != 0) {
91- fbs = true;
92- use_arb_ = true;
93+ if (use_arb) {
94 glGenFramebuffers(1, &gl_framebuffer_id_);
95 glBindFramebuffer(GL_FRAMEBUFFER, 0);
96- } else if (strstr(extensions, "GL_EXT_framebuffer_object") != 0) {
97- fbs = true;
98- use_arb_ = false;
99+ } else {
100 glGenFramebuffersEXT(1, &gl_framebuffer_id_);
101 glBindFramebufferEXT(GL_FRAMEBUFFER, 0);
102 }
103- if (!fbs) {
104- throw wexception
105- ("No support for GL_ARB_framebuffer_object or GL_ARB_framebuffer_object "
106- "in OpenGL implementation. One of these is needed for Widelands in OpenGL mode. You can "
107- "try launching Widelands with --opengl=0.");
108- }
109 }
110
111 /**
112
113=== modified file 'src/graphic/render/gl_surface_texture.h'
114--- src/graphic/render/gl_surface_texture.h 2013-02-10 17:39:24 +0000
115+++ src/graphic/render/gl_surface_texture.h 2013-07-14 11:00:38 +0000
116@@ -27,7 +27,7 @@
117 public:
118 // Call this once before using any instance of this class and Cleanup once
119 // before the program exits.
120- static void Initialize();
121+ static void Initialize(bool use_arb);
122 static void Cleanup();
123
124 GLSurfaceTexture(SDL_Surface * surface, bool intensity = false);

Subscribers

People subscribed via source and target branches

to status/vote changes: