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 format. > 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 before. > 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 packages? > > 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 :) Thanks! -- Martin Erik Werner <martinerikwerner@gmail.com>
Attachment:
signature.asc
Description: This is a digitally signed message part