> 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.
(...)
> > 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've now added copyright headers globally throughout the source files.
> > 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.
done
> > 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.
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.
>
All three removed, unused anyways.
(...)
> > 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..
This is unchanged for now.
> > 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.
Both added, along with hardening build flags and a debug package.
> > 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 :)
All these should be addressed now.
I've made a new upstream release, as per NEWS:
Version 0.2 - April 2012, by Martin Erik Werner
* New dependency on boost_program_options
* Reworked command-line handling completely
+ Generalised options
- -f --file : input file
- -d --dir : input dir
- -w --write : output file
- -h --help
- -v --sdlvideo
- arg1 is auto-assigned to input file
- arg2 is auto-assigned to input dir
- arg3 is auto-assigned to output file
* Fixed a bug where giving a directory path without the ending "/" would
set
the path to the parent directory instead.
* Defaults to reading data/liero.cfg instead of ./liero.cfg
* Removal of non-free dtoa piece in gvl
* Better resolution handling (only for 'Nearest' filter)
+ Game window now defaults to double the size (640x400)
+ Fullscreen now defaults to highest supported screen resolution
- Game area will grab max multiple of 320x200, rest becomes border
+ Added F7 and F8 keys for x3 and x4 scaling respecively
* Added ability to quit game via desktop events (window close, alt+F4,
etc.)
* Made configure checks for SDL_mixer not depend on pkg-config
* Error messages from handling config files are now much more verbose
+ If possible gives error type, file, line number, setting path, etc.
+ This is only enabled with libconfig++ version 1.4 and above
- (libconfig++9 in Debian/Ubuntu)
>
> IIRC with X11 there are hints you can send to the WM to prevent users
> from being able to resize to particular sizes, that might be useful to
> get rid of the bordering.
The problem here is actually that only integer multiples of 320x200
works for the currently used Nearest filter, I've used this with the
scaling keys and with fullscreen, so it's at least reasonable.
Arbitrary window drag-resizing still doesn't work, it appears that
somehow, SDL stops supplying SDL_RESIZEVIDEO events after a tenth of a
second of resizing or so, I've not figured out how to solve this...
> Also, I encountered a couple of segfaults when resizing, one in
> scale2x mode when resizing the window less than
> 320x200. The other was random while scaling in scale2x mode.
Yeah, the scale2x is something I've tried and failed at poking at, for
now it's stuck between 320x200 and 640x400, and frankly I personally
think that it's a disgrace to the classic pixly feel of Liero, so yeah,
it will probably not get that much love from me :)
New version is up on mentors:
http://mentors.debian.net/package/lierolibre
dget -x http://mentors.debian.net/debian/pool/main/l/lierolibre/lierolibre_0.2-1.dsc
And in git:
http://anonscm.debian.org/gitweb/?p=pkg-games/lierolibre.git;a=summary
git://anonscm.debian.org/pkg-games/lierolibre.git
I'd be happy is someone could re-review the package and sponsor it if it looks ok.
Thanks.
--
Martin Erik Werner <martinerikwerner@gmail.com>
Attachment:
signature.asc
Description: This is a digitally signed message part