Merge lp:~mxsscott/widelands/windowed-graphics into lp:widelands

Proposed by Mark Scott on 2013-01-06
Status: Merged
Merged at revision: 6484
Proposed branch: lp:~mxsscott/widelands/windowed-graphics
Merge into: lp:widelands
Diff against target: 414 lines (+138/-75)
8 files modified
src/sound/sound_handler.cc (+2/-2)
src/ui_fsmenu/base.cc (+2/-2)
src/ui_fsmenu/options.cc (+76/-24)
src/ui_fsmenu/options.h (+19/-6)
src/wlapplication.cc (+25/-4)
src/wlapplication.h (+12/-2)
src/wui/interactive_base.cc (+2/-32)
src/wui/interactive_base.h (+0/-3)
To merge this branch: bzr merge lp:~mxsscott/widelands/windowed-graphics
Reviewer Review Type Date Requested Status
SirVer 2013-01-06 Approve on 2013-01-07
Review via email: mp+142048@code.launchpad.net

Description of the change

Fix resizing of the game window after the user chooses a new resolution bug 1096651.

- Also menus use the current graphics dimensions, not the dimensions in the options (not really a problem now!)

- Some graphics resolution tidyups.

- Support Enter/Escape for triggering the Apply or Cancel buttons within the Options menu. This allows the user to reselect a smaller window size if they get stuck due to bug 900784.

The screen resolution issue fix could possibly be improved if we display a window for 5-10 seconds asking the user to confirm they can see it. If they don't click OK, we revert the graphics mode back to the previous one. However, I believe the Enter/Escape solution is 'good enough' for now.

To post a comment you must log in.
SirVer (sirver) wrote :

Nice change in total. The only issue I have is const SDL_Rect * const *. Maybe it's because i am a bad programmer, but i do not even know what type this is... a const pointer to pointer to constant SDL_Rects? does const SDL_Rect** compile without casting?

I also agree with the struct -> class changes and the simplifications concerning pulling the resolutions from the conf file periodically vs taking the real resolution.

I think here

 for (/* init above */; modes && *modes; ++modes)

you could just pull the initialization into the for loop as you do not do anything with the variable anyways.

otherwise looks good to me. Btw, I really enjoy reviewing your code :-). In the past we have always granted push to trunk rights to developers who have made good contributions. Are you interested in this? Then, we also no longer did code reviews and devs just push whatever they think is appropriate - however i am interested in changing this because I'd love to have code review for my own stuff as well in the future. .... this kinda went offtopic of this merge - I'd like to hear the comments of everybody though!

review: Approve
Mark Scott (mxsscott) wrote :

Reading right to left, it's a pointer to a const pointer to a const SDL_Rect.

const SDL_Rect** should also work because that's what SDL_ListModes() returns (we declare our variable to be more consty to indicate we don't mess with it.

The reason I left the init out of the for loop was due to line-length, but this is a pretty poor reason to do so.

I'll make these changes and re-push the branch.

I'm happy having trunk rights - but I do see the benefit of having code reviewed, especially where it is an area people are touching significantly or haven't touched before. Even if the coding is fine, there may be opportunity to say "Why don't you do this too?", or point out something that was missed "You left a todo in for function X's documentation" - we all forget these in the haste to commit before going away to do something else (sleep?)

6482. By Mark Scott on 2013-01-07

Changes post code-review - move SDL_ListModes() into for loop initializer.

Mark Scott (mxsscott) wrote :

Wasn't able to change the type
   const SDL_Rect * const *
to
   const SDL_Rect * *.

SDL_ListModes() returns a "SDL_Rect **" so we need to say we'll keep all but the final pointer variable constant (which is fine because that variable is ours), or nothing constant.

SirVer (sirver) wrote :

Okay, Ack. Code LGTM, so feel free to push it.

I gave you push rights to trunk (by adding you to widelands dev). here comes the very important usual disclaimer: bzr is awefull in one regard that you can push your branch history to trunk without confirmation. that is, if you work in a branch and someone adds new revisions to trunk in the meantime, when you push your branch to trunk you will overwrite those changes!!!

so, the proper thing is to always keep a version of trunk around that you do not modify. I personally use this workflow: http://www.taoeffect.com/blog/2009/06/using-bazaar-like-git-repoalias-plugin/

you then always work in branches:

$ cd trunk && bzr switch -b bug123
hack hack, commit, commit
$ bzr switch trunk && bzr pull && bzr merge .bzr-repo/bug123
$ bzr push lp:widelands

especially never push to lp:widelands from a feature branch or before you pulled. bzr uncommit helps sometimes to undo errors.

feel free to ask for anything when you need help. this is really important to get right - though basically everybody gets it wrong once. this will result in mailbombs from bzr and "XXX revisions removed." mails which are confusing at best.

Hans Joachim Desserud (hjd) wrote :

>I'd love to have code review for my own stuff as well in the future. .... this kinda went offtopic of this merge - I'd like to hear the comments of everybody though!

+1
Code reviews are really nice as they can often reveal and resolve issues before they hit trunk. And if something breaks in the future, both the author and reviewer should be familiar with the code as opposed to only a single person.

One potential issue I see is of shortage of reviewers. We currently have nine branches waiting for review, some of which are quite old. On the other hand, some of those have raised issues which authors haven't addressed yet.

I think this would be worth a try. We can always re-assess later if we end up with lots of unmerged reviews and a trunk gathering dust.

And since this is completely off-topic, we should find some better place to discuss it; a forum thread, meta bug?

SirVer (sirver) wrote :

right, maybe we should take this to widelands-public which is still the goto place to discuss dev topics.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/sound/sound_handler.cc'
2--- src/sound/sound_handler.cc 2013-01-04 16:39:48 +0000
3+++ src/sound/sound_handler.cc 2013-01-07 20:42:25 +0000
4@@ -330,8 +330,8 @@
5 Interactive_Base const & ibase = *m_egbase->get_ibase();
6 Point const vp = ibase.get_viewpoint();
7
8- int32_t const xres = ibase.get_xres();
9- int32_t const yres = ibase.get_yres();
10+ int32_t const xres = g_gr->get_xres();
11+ int32_t const yres = g_gr->get_yres();
12
13 MapviewPixelFunctions::get_pix(m_egbase->map(), position, sx, sy);
14 sx -= vp.x;
15
16=== modified file 'src/ui_fsmenu/base.cc'
17--- src/ui_fsmenu/base.cc 2013-01-05 21:03:10 +0000
18+++ src/ui_fsmenu/base.cc 2013-01-07 20:42:25 +0000
19@@ -86,11 +86,11 @@
20
21
22 uint32_t Fullscreen_Menu_Base::gr_x() {
23- return g_options.pull_section("global").get_int("xres", XRES);
24+ return g_gr->get_xres();
25 }
26
27 uint32_t Fullscreen_Menu_Base::gr_y() {
28- return g_options.pull_section("global").get_int("yres", YRES);
29+ return g_gr->get_yres();
30 }
31
32
33
34=== modified file 'src/ui_fsmenu/options.cc'
35--- src/ui_fsmenu/options.cc 2012-12-16 19:08:53 +0000
36+++ src/ui_fsmenu/options.cc 2013-01-07 20:42:25 +0000
37@@ -236,37 +236,47 @@
38
39 // GRAPHIC_TODO: this shouldn't be here List all resolutions
40 // take a copy to not change real video info structure
41- SDL_PixelFormat fmt = *SDL_GetVideoInfo()->vfmt;
42+ SDL_PixelFormat fmt = *SDL_GetVideoInfo()->vfmt;
43+
44 fmt.BitsPerPixel = 16;
45- if
46- (SDL_Rect const * const * const modes =
47- SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN))
48- for (uint32_t i = 0; modes[i]; ++i)
49- if (640 <= modes[i]->w and 480 <= modes[i]->h) {
50- res const this_res = {modes[i]->w, modes[i]->h, 16};
51+ for
52+ (const SDL_Rect * const * modes = SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN);
53+ modes && *modes;
54+ ++modes)
55+ {
56+ const SDL_Rect & mode = **modes;
57+ if (640 <= mode.w and 480 <= mode.h)
58+ {
59+ const Resolution this_res = {mode.w, mode.h, fmt.BitsPerPixel};
60 if
61 (m_resolutions.empty()
62- or
63- this_res.xres != m_resolutions.rbegin()->xres
64- or
65- this_res.yres != m_resolutions.rbegin()->yres)
66+ || this_res.xres != m_resolutions.rbegin()->xres
67+ || this_res.yres != m_resolutions.rbegin()->yres)
68+ {
69 m_resolutions.push_back(this_res);
70 }
71+ }
72+ }
73+
74 fmt.BitsPerPixel = 32;
75- if
76- (SDL_Rect const * const * const modes =
77- SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN))
78- for (uint32_t i = 0; modes[i]; ++i)
79- if (640 <= modes[i]->w and 480 <= modes[i]->h) {
80- res const this_res = {modes[i]->w, modes[i]->h, 32};
81- if
82- (m_resolutions.empty()
83- or
84- this_res.xres != m_resolutions.rbegin()->xres
85- or
86- this_res.yres != m_resolutions.rbegin()->yres)
87- m_resolutions.push_back(this_res);
88+ for
89+ (const SDL_Rect * const * modes = SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN);
90+ modes && *modes;
91+ ++modes)
92+ {
93+ const SDL_Rect & mode = **modes;
94+ if (640 <= mode.w and 480 <= mode.h)
95+ {
96+ const Resolution this_res = {mode.w, mode.h, fmt.BitsPerPixel};
97+ if
98+ (m_resolutions.empty()
99+ || this_res.xres != m_resolutions.rbegin()->xres
100+ || this_res.yres != m_resolutions.rbegin()->yres)
101+ {
102+ m_resolutions.push_back(this_res);
103 }
104+ }
105+ }
106
107 bool did_select_a_res = false;
108 for (uint32_t i = 0; i < m_resolutions.size(); ++i) {
109@@ -343,6 +353,26 @@
110 }
111 }
112
113+bool Fullscreen_Menu_Options::handle_key(bool down, SDL_keysym code)
114+{
115+ if (down)
116+ {
117+ switch (code.sym)
118+ {
119+ case SDLK_KP_ENTER:
120+ case SDLK_RETURN:
121+ end_modal(static_cast<int32_t>(om_ok));
122+ return true;
123+ case SDLK_ESCAPE:
124+ end_modal(static_cast<int32_t>(om_cancel));
125+ return true;
126+ default:
127+ break; // not handled
128+ }
129+ }
130+
131+ return Fullscreen_Menu_Base::handle_key(down, code);
132+}
133
134 Options_Ctrl::Options_Struct Fullscreen_Menu_Options::get_values() {
135 const uint32_t res_index = m_reslist.selection_index();
136@@ -561,6 +591,28 @@
137 }
138 }
139
140+bool Fullscreen_Menu_Advanced_Options::handle_key(bool down, SDL_keysym code)
141+{
142+ if (down)
143+ {
144+ switch (code.sym)
145+ {
146+ case SDLK_KP_ENTER:
147+ case SDLK_RETURN:
148+ end_modal(static_cast<int32_t>(om_ok));
149+ return true;
150+ case SDLK_ESCAPE:
151+ end_modal(static_cast<int32_t>(om_cancel));
152+ return true;
153+ default:
154+ break; // not handled
155+ }
156+ }
157+
158+ return Fullscreen_Menu_Base::handle_key(down, code);
159+}
160+
161+
162 Options_Ctrl::Options_Struct Fullscreen_Menu_Advanced_Options::get_values() {
163 // Write all remaining data from UI elements
164 os.message_sound = m_message_sound.get_state();
165
166=== modified file 'src/ui_fsmenu/options.h'
167--- src/ui_fsmenu/options.h 2012-02-15 21:25:34 +0000
168+++ src/ui_fsmenu/options.h 2013-01-07 20:42:25 +0000
169@@ -32,10 +32,11 @@
170 #include <cstring>
171 #include <vector>
172
173-struct Fullscreen_Menu_Options;
174+class Fullscreen_Menu_Options;
175 struct Section;
176
177-struct Options_Ctrl {
178+class Options_Ctrl {
179+public:
180 struct Options_Struct {
181 int32_t xres;
182 int32_t yres;
183@@ -80,7 +81,8 @@
184 * Fullscreen Optionsmenu. A modal optionsmenu
185 */
186
187-struct Fullscreen_Menu_Options : public Fullscreen_Menu_Base {
188+class Fullscreen_Menu_Options : public Fullscreen_Menu_Base {
189+public:
190 Fullscreen_Menu_Options(Options_Ctrl::Options_Struct opt);
191 Options_Ctrl::Options_Struct get_values();
192 enum {
193@@ -89,6 +91,9 @@
194 om_restart = 2
195 };
196
197+ /// Handle keypresses
198+ virtual bool handle_key(bool down, SDL_keysym code);
199+
200 private:
201 uint32_t m_vbutw;
202 uint32_t m_butw;
203@@ -127,19 +132,24 @@
204
205 void advanced_options();
206
207- struct res {
208+ /// A screen resolution in terms of width, height and pixel depth.
209+ class Resolution {
210+ public:
211 int32_t xres;
212 int32_t yres;
213 int32_t depth;
214 };
215- std::vector<res> m_resolutions;
216+
217+ /// All supported screen resolutions.
218+ std::vector<Resolution> m_resolutions;
219 };
220
221 /**
222 * Fullscreen Optionsmenu. A modal optionsmenu
223 */
224
225-struct Fullscreen_Menu_Advanced_Options : public Fullscreen_Menu_Base {
226+class Fullscreen_Menu_Advanced_Options : public Fullscreen_Menu_Base {
227+public:
228 Fullscreen_Menu_Advanced_Options(Options_Ctrl::Options_Struct opt);
229 Options_Ctrl::Options_Struct get_values();
230 enum {
231@@ -147,6 +157,9 @@
232 om_ok = 1
233 };
234
235+ /// Handle keypresses
236+ virtual bool handle_key(bool down, SDL_keysym code);
237+
238 private:
239 uint32_t m_vbutw;
240 uint32_t m_butw;
241
242=== modified file 'src/wlapplication.cc'
243--- src/wlapplication.cc 2013-01-04 15:24:56 +0000
244+++ src/wlapplication.cc 2013-01-07 20:42:25 +0000
245@@ -263,7 +263,7 @@
246 m_mouse_locked (0),
247 m_mouse_compensate_warp(0, 0),
248 m_should_die (false),
249-m_gfx_w(0), m_gfx_h(0),
250+m_gfx_w(0), m_gfx_h(0), m_gfx_bpp(0),
251 m_gfx_fullscreen (false),
252 m_gfx_opengl (true),
253 m_default_datadirs (true),
254@@ -790,11 +790,11 @@
255 */
256
257 void WLApplication::init_graphics
258- (int32_t const w, int32_t const h, int32_t const bpp,
259- bool const fullscreen, bool const opengl)
260+ (const int32_t w, const int32_t h, const int32_t bpp,
261+ const bool fullscreen, const bool opengl)
262 {
263 if
264- (w == m_gfx_w && h == m_gfx_h &&
265+ (w == m_gfx_w && h == m_gfx_h && bpp == m_gfx_bpp &&
266 fullscreen == m_gfx_fullscreen &&
267 opengl == m_gfx_opengl)
268 return;
269@@ -804,6 +804,7 @@
270
271 m_gfx_w = w;
272 m_gfx_h = h;
273+ m_gfx_bpp = bpp;
274 m_gfx_fullscreen = fullscreen;
275 m_gfx_opengl = opengl;
276
277@@ -815,6 +816,23 @@
278 }
279 }
280
281+void WLApplication::refresh_graphics()
282+{
283+ Section & s = g_options.pull_section("global");
284+
285+ // Switch to the new graphics system now, if necessary.
286+ init_graphics
287+ (s.get_int("xres", XRES),
288+ s.get_int("yres", YRES),
289+ s.get_int("depth", 16),
290+ s.get_bool("fullscreen", false),
291+#if USE_OPENGL
292+ s.get_bool("opengl", true));
293+#else
294+ false);
295+#endif
296+}
297+
298 /**
299 * Read the config file, parse the commandline and give all other internal
300 * parameters sensible default values
301@@ -1515,6 +1533,9 @@
302 std::string message;
303
304 for (;;) {
305+ // Refresh graphics system in case we just changed resolution.
306+ refresh_graphics();
307+
308 Fullscreen_Menu_Main mm;
309
310 if (message.size()) {
311
312=== modified file 'src/wlapplication.h'
313--- src/wlapplication.h 2012-02-15 21:25:34 +0000
314+++ src/wlapplication.h 2013-01-07 20:42:25 +0000
315@@ -179,8 +179,15 @@
316 //@}
317
318 void init_graphics
319- (int32_t w, int32_t h, int32_t bpp,
320- bool fullscreen, bool opengl);
321+ (const int32_t w, const int32_t h, const int32_t bpp,
322+ const bool fullscreen, const bool opengl);
323+
324+ /**
325+ * Refresh the graphics from the latest options.
326+ *
327+ * \note See caveats for \ref init_graphics()
328+ */
329+ void refresh_graphics();
330
331 void handle_input(InputCallback const *);
332
333@@ -282,6 +289,9 @@
334 ///The Widelands window's height in pixels
335 int32_t m_gfx_h;
336
337+ ///The Widelands window's bits per pixel
338+ int32_t m_gfx_bpp;
339+
340 ///If true Widelands is (should be, we never know ;-) running
341 ///in a fullscreen window
342 bool m_gfx_fullscreen;
343
344=== modified file 'src/wui/interactive_base.cc'
345--- src/wui/interactive_base.cc 2012-12-16 14:29:46 +0000
346+++ src/wui/interactive_base.cc 2013-01-07 20:42:25 +0000
347@@ -71,7 +71,7 @@
348 Interactive_Base::Interactive_Base
349 (Editor_Game_Base & the_egbase, Section & global_s)
350 :
351- Map_View(0, 0, 0, get_xres(), get_yres(), *this),
352+ Map_View(0, 0, 0, global_s.get_int("xres", XRES), global_s.get_int("yres", YRES), *this),
353 m_show_workarea_preview(global_s.get_bool("workareapreview", true)),
354 m
355 (new InteractiveBaseInternals
356@@ -112,15 +112,7 @@
357 (global_s.get_bool("dock_windows_to_edges", false));
358
359 // Switch to the new graphics system now, if necessary.
360- WLApplication::get()->init_graphics
361- (get_xres(), get_yres(),
362- global_s.get_int("depth", 16),
363- global_s.get_bool("fullscreen", false),
364-#if USE_OPENGL
365- global_s.get_bool("opengl", true));
366-#else
367- false);
368-#endif
369+ WLApplication::get()->refresh_graphics();
370
371 // Having this in the initializer list (before Sys_InitGraphics) will give
372 // funny results.
373@@ -238,28 +230,6 @@
374
375
376 /**
377- * Retrieves the configured in-game resolution.
378- *
379- * \note For most purposes, you should use \ref Graphic::get_xres instead.
380- */
381-int32_t Interactive_Base::get_xres()
382-{
383- return g_options.pull_section("global").get_int("xres", XRES);
384-}
385-
386-
387-/**
388- * Retrieves the configured in-game resolution.
389- *
390- * \note For most purposes, you should use \ref Graphic::get_yres instead.
391- */
392-int32_t Interactive_Base::get_yres()
393-{
394- return g_options.pull_section("global").get_int("yres", YRES);
395-}
396-
397-
398-/**
399 * Called by \ref Game::postload at the end of the game loading
400 * sequence.
401 *
402
403=== modified file 'src/wui/interactive_base.h'
404--- src/wui/interactive_base.h 2012-12-13 10:41:22 +0000
405+++ src/wui/interactive_base.h 2013-01-07 20:42:25 +0000
406@@ -113,9 +113,6 @@
407 virtual void cleanup_for_load() {};
408
409 private:
410- static int32_t get_xres();
411- static int32_t get_yres();
412-
413 void roadb_add_overlay ();
414 void roadb_remove_overlay();
415

Subscribers

People subscribed via source and target branches

to status/vote changes: