Re: Games::Frozenbubble is on CPAN

Kartik Thakore

On 2010-05-10, at 7:56 AM, Guillaume Cottenceau <[email protected]>  
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