[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Bug#670195: RFS: lierolibre/0.1-1

On Tue, 2012-04-24 at 10:14 +0800, Paul Wise wrote:
> On Tue, Apr 24, 2012 at 5:26 AM, Martin Erik Werner wrote:
> > I am looking for a sponsor for my new package "lierolibre"
> A review, since you are upstream too, I'm including some advice
> related to that too.
Thanks for the review, >> ./TODO :)

> I would suggest using git2cl or 'git log' upstream to create the ChangeLog file.
Ok, I've used "git log --stat", that seems to be the most readable

> I note many files don't have copyright/license headers:
> http://tieguy.org/blog/2012/03/17/on-the-importance-of-per-file-license-information/
I'm aware, I have taken to practice adding copyright headers to all
*header* files whose related code I poke in, but since I have not made
changes to all bits, many remain unchanged.

> I would suggest moving the C++ code to a src/ (or similar) subdir.
Yeah, that's probably a good idea, I'll look into that.

> I would suggest deleting sdl.m4 from your VCS and just letting
> autotools copy in the one from SDL.
Via autoreconf you mean? I'll look into that.

> How about completely dropping the zlib copy from your gvl fork?
Yeah, true, ancient anyways, done.

> gvl is both an embedded code copy and a nest of embedded code copies,
> some of which are even in Debian (zlib, libpcl1-dev, libtut-dev). My
> eyes!
Well the zlib is unused, (and now deleted). I was not aware of libpcl
and libtut, I'll have a look at ripping those out as well.

> I found this post about dtoa.c:
> http://patrakov.blogspot.com.au/2009/03/dont-use-old-dtoac.html
Hmm, I though I removed that since I found it non-free... Anyways, it's
not used in any of the resulting binaries, and now deleted.

> Hmm, you are using both the jam and autotools build systems, is that on purpose?
I am keeping the Jam build updated and working, so that both may be
used, it's not being used in the packaging build, if I haven't made some
clear mistake...

> I'd suggest that individual graphics in separate .xpm files would be a
> better way to develop the graphics than strips of multiple graphics in
> one .xpm file.
Maybe, and mapping the palette somehow would also be handly, it's a
TODO, but the current method is at least better than nothing, as noted

> You might want to add the old liero release info to NEWS?
True, done so.

> On my screen the default window size is so small I can't read the
> writing. How about making it 800x600?
> I would suggest using one of these algorithms for window scaling:
> https://en.wikipedia.org/wiki/Image_scaling
> http://research.microsoft.com/en-us/um/people/kopf/pixelart/supplementary/index.html
Yeah, the native game resolution is 320x200 pixels, and anything else is
scaled. Both nearest and scale2X are available currently, toggled via
the [F1] in-game menu.

I think I've got a reasonable hack setting it to 800x500 (x2.5) for now,
though the code that handles the resolution is somewhat of a mystery to
me at the moment.

One can also use:
window-managers default maximise method (alt+F10, double-click title,
maximise button)
F6 to get 640x480, scaled
F5 to get fullscreen, scaled

Unfortunately all higher resolutions seem to add unneeded bordering...

> I would suggest changing the default keyboard settings for player 1 to
> the more standard wasd.
I disagree on this, Since control, alt and shift are used wasd becomes
very cumbersome. For singleplayer I personally use ijkl and asd for
actions, but that's also quite odd, and inappropriate for splitscreen,
so I think it makes best sense to keep the LIERO default here.

> The change weapon button doesn't seem to work.
Do you mean the one for Player2? Yeah, that's the default case for me
as well, though I suspect that that's due to me having Alt Gr instead
of LAlt, and I'm not sure if it's possible to support both, I'll have
to look into that..

> Why the dpkg predepends?
Hmm, since I am (or at least should be) using xz compression for the

> Please use dh --parallel or mention in debian/rules why build
> parallelism isn't possible to use.
Ah, ok, I'll do that.

> I would suggest depending on dh-autoreconf and running dh --with
> autoreconf to ensure that the configure script and Makefiles are
> always buildable on Debian. This is useful since Debian does rebuild
> testing for newer versions of autotools.
Ah, I haven't looked at autotools from a Debian perspective enough it
seems, thanks for the hint.

> cppcheck warnings:
> [gvl/gvl_test/_build/deque.cpp:51]: (error) Throwing exception in destructor
> [gvl/crypt/curve25519.cpp:318]: (error) Memory leak: temp
> [gvl/containers/tests/deque.cpp:48]: (error) Throwing exception in destructor
> [gvl/crypt/curve25519.cpp:690]: (error) Memory leak: temp1
> [gvl/uthread/uthread.cpp:100]: (error) Throwing exception in destructor
> [viewport.cpp:132]: (error) Possible null pointer dereference: worm -
> otherwise it is redundant to check if worm is null at line 111

I'll have to do some digging here :)


Martin Erik Werner <martinerikwerner@gmail.com>

Attachment: signature.asc
Description: This is a digitally signed message part

Reply to: