On Fri, May 07, 2010 at 04:07:17PM +0200, Guillaume Cottenceau wrote:
> >> 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!
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;
Good Catch! I have changed this.
> 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?
>
I will work on this with FROGGS. Btw we converted the one mikmod file to OGG.
The *.xm one. The header for that file was giving us a lot of trouble.
>
> - $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?)
Yes. But I think we should have indiviual tests for this stuff. Maybe
when we refactor.
>
> - 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?
Yes. I don't know why you had the '$$' but it doesn't work like that.
The $ticks is now just an integer.
>
>
> + printf("print_: " . join(' ', (caller(1))) . " $text\n");
>
> a debug statement I guess
You guessed it.
>
> +
> SDL::Video::wm_set_icon(SDL::Video::load_BMP("$FPATH/gfx/pinguins/window_icon_penguin.png"));
>
> load_"BMP" for a PNG looks strange
Icons can only be set with BMP on windows. We need to change that. I
will make a note of it to use a .bmp file for windows.
http://sdlperl.ath.cx/projects/SDLPerl/ticket/135
>
> #- 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
It should. Again tests might help here.
>
> 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.
Good catch I will change this.
http://sdlperl.ath.cx/projects/SDLPerl/ticket/136
>
> #- 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
Added to ticket 136
>
>
> - 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?
No the highscores should work. Wait what branch are you on? Redesign?
This was changed later.
>
>
> - 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.
Good point. We should change this in the refactor or before.
>
>
> - 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?
I don't understand the breakage here. Do we need this? I think this was
FROGGS fix.
>
> - 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?
No I didn't test farsi. Again FROGGS knows more about this.
>
> 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
...
>
> $$back -> $back behavorial change?
Yup no more crazy referal things.
>
> I need to stop now; I'll try to review the other half soon!
>
>
> PS: Anyone tested the joysticks?
No don't have one. Please test it?
> 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
I have update the Games::FrozenBubble to 2.202 as you said.
P.S.
Please join us on #sdl irc.perl.org it makes this much eaiser.
>
> --
> Guillaume Cottenceau - http://zarb.org/~gc/
Peace,
Kartik
