>> 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/
