>> I did a git rebase between your master and our redesign branch to make >> the new integration branch. And it has your commits >> >> http://github.com/kthakore/frozen-bubble/commits/integration?page=4 > > I'll try to look at that soon!
My approach to reviewing: instead of reviewing each commit separately, which would make little sense because of the previous works in progress and because I don't know well the new API yet, I am trying to review a diff of the latest state of my git repository compared to yours. I can tell my observations when they make sense. Then I propose in a second step, I can try to run the game and check the features. Thanks! Overally, it seems pretty good. Here are a few observations: bin/frozen-bubble 0f86ce221a71d95474648e2442cf3187989b390b -$sdl_flags = SDL_ANYFORMAT | SDL_HWSURFACE | SDL_DOUBLEBUF | SDL_HWACCEL | SDL_ASYNCBLIT; +$sdl_flags = SDL_HWSURFACE | SDL_DOUBLEBUF | SDL_HWACCEL | SDL_ASYNCBLIT; +$sdl_flags = SDL_SWSURFACE; redefining looks suspicious - $mixer = eval { SDL::Mixer->new(-frequency => 44100, -channels => 2, -size => 1024); }; + eval { + SDL::Mixer::open_audio( 22050, AUDIO_S16SYS, 2,1024); + }; (indentation and spaces suck) I used to use 44100 as frequency; I know that was something already used by debian but I am not sure what it really brings. normally, 22khz music is audibly less nice than 44khz. do you have some sort of reproduceable problems fixed by that change? - $sound{$_}->volume(80); + $sound{$_}->volume(100); I think it was 80 to properly balance music and sound effects volumes. Is that change really needed? Apparently initial previous change and this one is: commit b251d7479f36064496bd889c4b35c6af2e5e5eeb Author: Kartik Thakore <thakore.kar...@gmail.com> Date: Fri Oct 2 19:10:32 2009 -0400 Applied sound distort diff from here http://fb-win32.sourceforge.net/sound.diff diff --git a/frozen-bubble b/frozen-bubble index 8aa7c04..fae9d5b 100755 --- a/frozen-bubble +++ b/frozen-bubble @@ -299,7 +299,7 @@ sub play_music($) { } sub init_sound() { - $mixer = eval { SDL::Mixer->new(-frequency => 44100, -channels => 2, -size => 1024); }; + $mixer = eval { SDL::Mixer->new(-frequency => 22050, -channels => 2, -size => 1024); }; if ($@) { $@ =~ s| at \S+ line.*\n||; print STDERR "\nWarning: can't initialize sound (reason: $@).\n"; @@ -311,7 +311,7 @@ sub init_sound() { my $sound_path = "$FPATH/snd/$_.ogg"; $sound{$_} = SDL::Sound->new($sound_path); if (UNIVERSAL::isa($sound{$_}, 'HASH') ? $sound{$_}{-data} : ${$sound{$_}}) { - $sound{$_}->volume(80); + $sound{$_}->volume(100); } else { print STDERR "Warning, could not create new sound from '$sound_path'.\n"; } But I'm still wondering if it's really needed. Anyone witnessed sound distortion? Or is it happening only on Windows? Would that be a bug specific to the Windows platform in SDL or mikmod? - $save = SDL::Surface->new(-width => $image->width, -height => $image->height, -depth => 32, -Amask => "0 but true"); #- grrr... this piece of shit of Amask made the surfaces slightly modify along the print/erase of "Hurry" and "Pause".... took me so much time to debug and find that the problem came from a bug when Amask is set to 0xFF000000 (while it's -supposed- to be set to 0xFF000000 with 32-bit graphics!!) - $background->blit($drect, $save, $rects{$image}); + $save = SDL::Surface->new( SDL_ANYFORMAT, $image->w, $image->h, 32, 0 , 0, 0, 0); #- grrr... this piece of shit of Amask made the surfaces slightly modify along the print/erase of "Hurry" and "Pause".... took me so much time to debug and find that the problem came from a bug when Amask is set to 0xFF000000 (while it's -supposed- to be set to 0xFF000000 with 32-bit graphics!!) + SDL::Video::blit_surface($background, $drect, $save, $rects{$image}); my comment is probably outdated since "0 but true" for Amask disappeared? (did you test long-running Hurry/Pause overlay graphics and didn't notice any slight visual modification over time?) - if ($app->ticks - $$ticks > 1000) { - fb_net::gsend('p'); - $$ticks = $app->ticks; + if (SDL::get_ticks() - $ticks > 1000) { + Games::FrozenBubble::Net::gsend('p'); + $ticks = SDL::get_ticks(); changing $$ticks by $ticks modifies behaviour, was that really intended? + printf("print_: " . join(' ', (caller(1))) . " $text\n"); a debug statement I guess + SDL::Video::wm_set_icon(SDL::Video::load_BMP("$FPATH/gfx/pinguins/window_icon_penguin.png")); load_"BMP" for a PNG looks strange #- now crop (and use native RGBA ordering) - my $replace = SDL::Surface->new(-width => $canon{data}{$number}[2], -height => $canon{data}{$number}[3], -depth => 32); - $canon{img}{$number}->set_alpha(0, 0); - $canon{img}{$number}->blit(SDL::Rect->new('-x' => $canon{data}{$number}[0], '-y' => $canon{data}{$number}[1], - -width => $canon{data}{$number}[2], -height => $canon{data}{$number}[3]), $replace, undef); + my $replace = SDL::Surface->new( SDL_SWSURFACE, $canon{data}{$number}[2], $canon{data}{$number}[3], 32, 0xFF000000, 0xFF0000, 0xFF00, 0xFF); does this still use native RGBA ordering (I see 0xFF00000 etc specifications)? that may be slower on some architectures (ppc) if it's not native foreach my $cursortype (keys %{$imgbin{menu_cursor}}) { foreach my $img (@{$imgbin{menu_cursor}{$cursortype}}) { - my $alpha = SDL::Surface->new(-width => $img->width, -height => $img->height, -depth => 32); - $img->set_alpha(0, 0); #- for RGBA->RGBA blits, SDL_SRCALPHA must be removed or destination alpha is preserved - $img->blit(undef, $alpha, undef); - fb_c_stuff::alphaize(surf($alpha)); - push @{$imgbin{menu_cursor}{"${cursortype}alpha"}}, $alpha; - $img->set_alpha(SDL_SRCALPHA(), 0); + #my $alpha = SDL::Surface->new( SDL_ANYFORMAT, $img->w, $img->h, 32, 0,0,0,0); + #SDL::Video::set_alpha($img, 0, SDL_ALPHA_OPAQUE); #- for RGBA->RGBA blits, SDL_SRCALPHA must be removed or destination alpha is preserved + #SDL::Video::blit_surface($img, SDL::Rect->new(0,0,$img->w,$img->h), $alpha, SDL::Rect->new(0,0,$img->w,$img->h)); + #Games::FrozenBubble::CStuff::alphaize($alpha); + push @{$imgbin{menu_cursor}{"${cursortype}alpha"}}, $img; #$alpha; + #SDL::Video::set_alpha($img, SDL_SRCALPHA, SDL_ALPHA_OPAQUE); } } is that definitive? most of it is commented out. #- the RGBA effects algorithms assume little endian RGBA surfaces - my $replace = SDL::Surface->new(-width => $imgbin{menu_logo}->width, -height => $imgbin{menu_logo}->height, -depth => 32); - $imgbin{menu_logo}->set_alpha(0, 0); #- for RGBA->RGBA blits, SDL_SRCALPHA must be removed or destination alpha is preserved - $imgbin{menu_logo}->blit(undef, $replace, undef); - $imgbin{menu_logo} = $replace; - add_default_rect($replace); + #my $replace = SDL::Surface->new( SDL_ANYFORMAT, $imgbin{menu_logo}->w, $imgbin{menu_logo}->h, 32, 0, 0, 0, 0); + #SDL::Video::set_alpha($imgbin{menu_logo}, 0, SDL_ALPHA_OPAQUE); #- for RGBA->RGBA blits, SDL_SRCALPHA must be removed or destination alpha is preserved + #SDL::Video::blit_surface($imgbin{menu_logo}, SDL::Rect->new(0,0,$imgbin{menu_logo}->w,$imgbin{menu_logo}->h), $replace, SDL::Rect->new(0,0,$imgbin{menu_logo}->w,$imgbin{menu_logo}->h)); + #$imgbin{menu_logo} = $imgbin{menu_logo}; #$replace; + #add_default_rect($replace); same - my $tmp = SDL::Surface->new(-width => $high_rect->width/4, -height => $high_rect->height/4, - -depth => 32, -Amask => "0 but true")->display_format; - fb_c_stuff::shrink(surf($tmp), surf($background->display_format), 0, 0, rect($high_rect), 4); - $tmp->blit(undef, $app, SDL::Rect->new(-x => $high_posx, '-y' => $high_posy)); + my $tmp = SDL::Video::display_format(SDL::Surface->new($high_rect->w/4, $high_rect->h/4, 32, 0, 0, 0, 0)); + #Games::FrozenBubble::CStuff::shrink($tmp, SDL::Video::display_format($background), 0, 0, $high_rect, 4); + SDL::Video::blit_surface($tmp, SDL::Rect->new(0,0,$tmp->w,$tmp->h), $app, SDL::Rect->new($high_posx, $high_posy, $tmp->w, $tmp->h)); same - with shrinking disabled the highscores are probably broken? - if ($i) { - print_('menu', $app, $MENUPOS{xpos_panel}, $ypos, $i, $imgbin{void_panel}->width, 'center'); - } - $ypos += $lineheight; + print_('menu', $app, $MENUPOS{xpos_panel}, $ypos, $i, $imgbin{void_panel}->w, 'center') if $i; + $ypos += $lineheight; I think it's better to stick with the old code style (even if it's probably not perfect) than mixing different code styles; in particular, I avoid veeeeeeeryyyyyyyy looooongggggg statements following by a very short conditional (if $i) because it's easy to miss the conditional. So please I think it's not good to make such modifications. - my $synchro_ticks = $app->ticks; - if ($graphics_level > 1) { - $draw_overlook->(); - $app->update(@update_rects); - @update_rects = (); - } - $event->pump; - while ($event->poll != 0) { + my $synchro_ticks = SDL::get_ticks(); +# commented out because otherwise we see only a white box instead of highlighted background image +# if ($graphics_level > 1) { +# $draw_overlook->(); +# SDL::Video::update_rects($app, @update_rects); +# @update_rects = (); +# } + SDL::Events::pump_events(); + while (SDL::Events::poll_event($event) != 0) { where do we stand, about that one? any fix along the way? - if ($smg_status_messages[-$i] =~ /^<U+202B>/) { + if ($smg_status_messages[-$i] =~ /^?/) { #- if text begins with unicode RTL direction, we also need to tell pango to align on the right why was that changed? the "strange" character was here on purpose. have you tested in e.g. farsi? http://www.fileformat.info/info/unicode/char/202B/index.htm seems that change originates from: commit cf36fb9b66c568c542445a813783488695afff13 Author: Kartik Thakore <thakore.kar...@gmail.com> Date: Thu Apr 1 21:17:27 2010 +0000 Frozen Bubble on WINDOWSgit statusgit status sub save_back_spot { my ($latitude, $longitude, $surface, $back) = @_; my ($x, $y) = get_spot_location($latitude, $longitude); - $x -= $surface->width/2; - $y -= $surface->height/2; - $$back = SDL::Surface->new(-width => 20, -height => 20, -depth => 32, -Amask => '0 but true'); - $app->blit(SDL::Rect->new('-x' => $x, '-y' => $y, -width => 20, -height => 20), $$back, undef); - add_default_rect($$back); + $x -= $surface->w/2; + $y -= $surface->h/2; + $back = SDL::Surface->new(20, 20, 32, 0, 0, 0, 0); + SDL::Video::blit_surface($app, SDL::Rect->new($x, $y, 20, 20), $back, SDL::Rect->new(0,0,$back->w,$back->h)); + add_default_rect($back); } $$back -> $back behavorial change? I need to stop now; I'll try to review the other half soon! PS: Anyone tested the joysticks? PS: can you add the already discussed warning about not being an official release, or something else with the same intent, on CPAN? search still produces http://search.cpan.org/~kthakore/Games-FrozenBubble-2.201/lib/Games/FrozenBubble.pm which seems unmodified so far -- Guillaume Cottenceau - http://zarb.org/~gc/