Kartik Thakore

On 2010-05-10, at 7:56 AM, Guillaume Cottenceau <gcott...@gmail.com> wrote:

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.

Thanks.

Btw we converted the one mikmod file to OGG.
The *.xm one. The header for that file was giving us a lot of trouble.

Oh. That would mean a rough increase of 100% in size with no real
value, it's not too good. What was the trouble? Mikmod doesn't work
anymore?

Btw the 2 player game music seems to still be xm:

This is different in the redesign branch.
frozen-bubble/share/snd/frozen-mainzik-2p.xm

- $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.

Yes we should have more tests, but either we have tests then we can
rely on them for modifications, or we do modifications first and then
we have to manually test. I don't want a new release of FB with
regressions.

I did manually test this by starting a level and getting the 'Hurry' for each bubble launch. It worked fine.

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

Using "$$ticks" in that subroutine has nothing to do with sdlperl
internals. That code:

-=-=---=-=---=-=---=-=---=-=--
sub mp_ping_if_needed {
   my ($ticks) = @_;
   if ($app->ticks - $$ticks > 1000) {
       fb_net::gsend('p');
       $$ticks = $app->ticks;
   }
}

...

  my $ticks = $app->ticks;

  while (1) {
      mp_ping_if_needed(\$ticks);
      ...
  }
-=-=---=-=---=-=---=-=---=-=--

is broken with your $$ticks => $ticks modification. Do you see what's
happening now?

Yes I see 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

Fine, but does that line of code works on Linux?

Yes this works in linux.
-            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.

I use the branch "integration" as it was in your http URL earlier. Is
that incorrect?

No we are on the redesign brach now.

I doubt the highscores would work without the shrinking stuff
activated. It's how "screenshots" are rendered. Have you tested it
with a non empty highscores list?

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

I think that line of code should be changed back now :)
Ok.

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

The breakage is it seems you removed the overlook visual effect and it
needs to be fixed to avoid a regression. That visual effect doesn't
generate a white box with current stable FB.
In redesign this is fixed.

$$back -> $back behavorial change?
 Yup no more crazy referal things.

No, you missed the point - same as previous case.

Yup I get it now thx.
PS: Anyone tested the joysticks?
No don't have one. Please test it?

I'll test in a second phase, for the moment I want to finish reviewing
(and I don't have a joystick anymore).

Anyone else in your development team having a joystick?

I dont know.
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.

Thanks.

No problem.
Please join us on #sdl irc.perl.org it makes this much eaiser.

I have joined the channel but I'm only occasionally available there..

That is fine. But you can ask questions on there faster.

--
Guillaume Cottenceau - http://zarb.org/~gc/

Thank you so much for reviewing.

Kartim

Reply via email to