GNOME Bugzilla – Bug 664983
Review and update artwork
Last modified: 2014-09-16 13:39:25 UTC
Review and update the artwork and make sure it is modern and consistent with the other GNOME Games.
As part of the Vala rewrite the icons were updated to use new scalable versions. The sun and moon looks the nicest, but it is complex and slow to render and the sun is not sharp on the image. This should be improved.
Some thought needs to be put into the board the tiles are placed on as it's currently unclear how big the grid is and where the player is allowed to play.
I would like to work on this. I have spoken to gnome-design about 'mentoring' me with this, and possibly the board/grid layout problem also.
Some work by Allan Day: https://github.com/gnome-design-team/gnome-mockups/blob/master/games/iagno/iagno-wireframes.png
More work from Allan: https://raw.github.com/gnome-design-team/gnome-mockups/master/games/iagno/iagno.png
(In reply to comment #6) > More work from Allan: > https://raw.github.com/gnome-design-team/gnome-mockups/master/games/iagno/iagno.png Is this still the desired result? I see there is one more svg with different menu and play again button. If so, I'd give it a try.
Yeah, that screenshot is still the desired result, though it's missing the undo button in the header bar (which is already fine).
When should the side menu should be displayed? When pressing "Start over"? And what about the current Preferences menu and menu entry?
We're discussing that in bug #664976; let's leave this bug for just the artwork.
Created attachment 286166 [details] [review] Update game information grid For now, I updated the game information grid on the top right. I'll see what else I can do that only involves the artwork. It's still one of my first patches and I'm sure I can do some things better. Can you take a look?
Created attachment 286173 [details] [review] Update background image. Also updated image according to the mockup
Created attachment 286174 [details] Screenshot with both patches applied.
Wow, good job! Digital art skills are valuable in the GNOME community, since we only have a few talented artists.
Review of attachment 286166 [details] [review]: Awesome; this turned out a lot better than my (unposted) attempt to do the same last year. The one thing I'm not totally sold on here is light.svg and dark.svg. In Allan's mockup, the same images are used in the scores table as in the game itself (they're just a bit smaller in the scores table). I think it'd look better to drop these separate images and use the normal game pieces instead. Right now, the numerical score of the player to move is displayed in bold. That was previously needed to indicate the player to move, but now that we have an arrow instead, both scores should always be bold. P.S. Don't be discouraged by the length of my comments below; this is actually pretty significant for your first contribution. ::: src/iagno.vala @@ +30,2 @@ private Gtk.Label dark_score_label; + private Gtk.Image light_icon; Can these variables be declared in a smaller scope? It looks like dark_icon and light_icon are only used in the function where you create them. @@ +159,3 @@ grid.vexpand = true; grid.hexpand = true; grid.set_column_spacing (8); I think 12 pixels looks better for column spacing. (And for some reason, we seem to like multiples of 6.) @@ +160,3 @@ grid.hexpand = true; grid.set_column_spacing (8); + grid.set_row_spacing (8); The mockup uses a lot more space here, and I think that looks better. Try 18 or 24 pixels, as you prefer. We also have an unfortunate alignment issue: shrink your window down to the minimum size, and you'll see there's a bit too much space between the board and the score area. We want to somehow reduce that without also reducing the space between the board and the Start Over button. I couldn't find a very clean way to do this, but setting the margin-end property of the grid to 12 seemed to work well (at least when combined with my other spacing recommendations). I figured these out by playing with GtkInspector: press Ctrl+Shift+D inside Iagno (with a recent GTK+ 3.13, like something built from jhbuild) and you can play with all the properties of your widgets to see easily what looks best. @@ +163,2 @@ grid.show (); side_box.pack_start (grid, false, true, 0); The 0 at the end is the amount of extra padding to add before the grid. I think it'd look a bit better to add 6 pixels, so that the top of the scoreboard seems to be slightly below the top of the grid, instead of slightly above. @@ +164,3 @@ side_box.pack_start (grid, false, true, 0); + var dark = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "dark.svg"); Be careful to always leave a space before the opening parenthesis. Also, GLib is imported by default, so don't specify this explicitly. It's assumed. @@ +173,3 @@ grid.attach (dark_score_label, 2, 0, 1, 1); + var light = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "light.svg"); Space before opening parenthesis, and don't explicitly specify GLib. @@ +322,3 @@ new_game_action.set_enabled (game.can_undo (1)); + var mark = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "mark.svg"); Space, GLib. @@ +323,3 @@ + var mark = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "mark.svg"); + mark_icon = new Gtk.Image.from_file (mark); Instead of re-creating the image each time, instead just two different images from the same file once (in the same place where you create dark_icon and light_icon), and here choose which one of those two to show and which to hide. @@ +329,3 @@ + grid.insert_column (0); + grid.attach (mark_icon, 0, 0, 1, 1); + mark_icon.show(); Space @@ +339,3 @@ + grid.insert_column (0); + grid.attach (mark_icon, 0, 1, 1, 1); + mark_icon.show(); Space
Review of attachment 286166 [details] [review]: Oh I found a bug. :) I guess we need to store the new light.svg, dark.svg, and mark.svg in a different directory, since they're being picked up as themes that you can pick in Preferences -- to very buggy effect!
Review of attachment 286166 [details] [review]: ::: data/Makefile.am @@ +10,3 @@ + dark.svg \ + light.svg \ + mark.svg So instead of putting these in theme_DATA, move them up out of the themes folder and list them under dist_DATA (instead of theme_DATA).
Review of attachment 286173 [details] [review]: I couldn't find anything wrong with this. It looks a lot better! For a follow-up patch, try thickening the lines of the grid, as shown in Allan's mockup.
Created attachment 286200 [details] [review] Update game information grid according to the review. I was expecting a longer review. It's better this way so I can make sure I learn the DOs and DON'Ts from the very beginning. Thanks for your supportive words.
Created attachment 286201 [details] [review] Update artwork and move tiles out of the themes directory. Please check this one, I'm not sure if what I did in the Makefile.am it's 100% ok.
Created attachment 286203 [details] Proof I tried setting the GRID_WIDTH in game-view.vala to 2 but the lines turn purplish. Setting it to like 8, makes them white + black + purple or something and I cant figure it why. All the formulas seem to be using GRID_WIDTH so the positioning should change accordingly.
Created attachment 286204 [details] [review] Update artwork and move tiles out of the themes directory. I forgot to add margin-end of 12. This should be ok now.
(In reply to comment #21) > I tried setting the GRID_WIDTH in game-view.vala to 2 but the lines turn > purplish. Setting it to like 8, makes them white + black + purple or something > and I cant figure it why. All the formulas seem to be using GRID_WIDTH so the > positioning should change accordingly. I guess we'll have to look more into this later. Thin is better than purple!
Review of attachment 286200 [details] [review]: ::: src/iagno.vala @@ +333,3 @@ /* Translators: this is a 2 digit representation of the current score. */ dark_score_label.set_markup ("<span font_weight='bold'>"+(_("%.2d").printf (game.n_dark_tiles))+"</span>"); + light_score_label.set_markup ("<span font_weight='bold'>"+(_("%.2d").printf (game.n_light_tiles))+"</span>"); These don't need to be inside the conditional anymore, right?
Review of attachment 286204 [details] [review]: Instead of attaching a separate patch for this, could you please merge it into the previous patch using git rebase -i? ::: data/Makefile.am @@ +5,3 @@ +distdir = $(datadir)/iagno +dist_DATA = \ The dist_prefix just says to distribute the files; you don't need to define distdir to use it. To be consistent with the rest of the file, how about: imagedir = $(datadir)/iagno image_DATA = \ ... And then you can include $(image_DATA) in EXTRA_DIST. @@ +37,3 @@ EXTRA_DIST = \ $(ui_DATA) \ + $(dist_DATA) \ And this is redundant if you're using the dist prefix.
Created attachment 286221 [details] [review] Update game information grid Done.
Review of attachment 286221 [details] [review]: Alright, great! I notice that if I switch the tile set to sun and star, the scoreboard still shows the pieces from the black and white tile set. I guess this would be not be super easy to fix, though. We could do it by creating a folder for each theme inside the theme directory, and giving each folder the main theme image, and also its own dark.svg and light.svg. You don't have to do that in this patch, though, if you'll file a bug for it. ::: src/iagno.vala @@ +151,2 @@ var side_box = new Gtk.Box (Gtk.Orientation.VERTICAL, 6); + side_box.margin_end = 12; Can you try adding this on the grid instead of side_box? I think this is causing the Start Over button to get pushed too close to the game board.
Created attachment 286227 [details] [review] Update game information grid Done. I will file a bug for the scoreboard-theme problem so I can treat that separately. You can assign it to me, I really want to continue contributing.
Review of attachment 286227 [details] [review]: Great! That's what I was hoping for. :)
Attachment 286173 [details] pushed as c42dbc0 - Update background image. Attachment 286227 [details] pushed as 6543481 - Update game information grid
I reopen, as there’s a little thing I dislike, concerning the new artworks of the information grid. The dark dot is quite cool, because it’s dark, so we don’t really see shadows. But the light dot has shadows, shadows that “reveals” it’s not a flat (reversible) piece of reversi, but a convex gravel of go… Allan’s designs for example didn’t have this problem. Probably just tweaking the gradient would be enough, but I didn’t test for now.
Ideally we would update the pieces to look like Allan's. Anyway, this is covered by bug #710125.