[FreeCiv-Java] Re: Upgrading to 1.10 and restructuring Client.java a bit

[Artur Biesiadowski <abies@xxxxxxxxx>]

Brian Duff wrote:

> I've been doing a bit of work in my free time with the freeciv java
> client source. I started off trying to get Revolutions to work properly,
> but then got a bit deeper into things and ended up upgrading the
> networking bits of the client to be compatible with a 1.10.0 server.
> I've also been thinking about ways to restructure Client.java slightly
> so that it doesn't become enormous and all-encompasing.

I'm the author of this code and I'm responsible for all bad design
decisions which are visible in freeciv-java :) 

Only part of client that I'm not ashamed of is org.freeciv.tile package.
Of course it could be done better, but I think it is quite extensible,
object oriented etc. This was the only part of code I wanted to use
outside the project and thus I spend some time thinking about
design/interface.
Rest of code is basically one great hack to stick freeciv client on top
of map code. Reason for Client being so large was to have a place where
all things meet - gui, networking, resources etc. I generally hate
static variables and just use Client as one big pseudo-global object to
keep all things that can come handy - thus you will see a lot places
where a reference to client is passed as argument.
There is too much code in it - for example now I would move tile
processing away from it (it will have to be done anyway to read new
tilespec - more on that later). One thing that I would like to defend is
keeping networking inside Client class - there is really no reason to
put it into other class, as all network traffic needs to be dispatched
to various places of system and only Client knows about them all.
One thing that comes to mind is placing small handling routines inside
net.Pkg* classes. This might be good way (Client reference would have to
be passed anyway, but there would be less code in one file), but there
is a lot of cases where same packet format is used for different
purposes, just by differenting type of packet. Having case statement
inside PkgGenericInteger is worse that current way and creating separate
Pkg* class for each type of request is a bit overkill. There is a way to
do it without code replication - having all Pkg classes using
PkgGenericInteger inherit from it and just reimplement handle(Client
client) method. I'm not convinced, but if you are really worried about
Client.java size you might consider it. Just remember that there would
be a big switch statement in Client anyway to create different types of
Packets. Only bnus would be moving away actual handle*() methods from
Client to Pkg class (allowing maybe to make a lot of Pkg fields
private).

> Is there a procedure in place for submitting code? It's still a bit
> early for me to check anything in, but I was just curious about how I
> would go about doing it when I'm ready.

I'm not longer actively developing the client, so if you really think
about working on it, I could handle it to you. For now 'official'
distribution site for source/data files is cvs server at www.gjt.org.
There is no problems in updating the source there. I've actually started
change to 1.9.0 some time ago, but I haven't finished it and thus it is
not in cvs (as I preferred to have something that works with _some_
server out there).


> Are there any automated tools for generating the java source?, e.g. I
> noticed that Constants.java has a "do not modify, machine generated"
> comment at the top, since I need to upgrade this for 1.10.0, it'd be
> nice not to have to do it by hand (I've been modifying it manually when
> I need to up till now). 
Code for it is inside org.freeciv.util.EnumParser. It should be written
in perl, but I don't know perl, so it ended up being in java. It is
extreme hack, I don't suppose it can parse various enums very well, but
it kind-of-worked for freeciv source. After generating there is some
manual work - there are few enums missing, some problems with terminator
in few places etc, so it is not fire-and-forget thing. I wanted
something to bootstrap Constants.java file and later I just edited it by
hand. I suppose that it is still faster/better to edit it by hand,
especially as you will need only few changed for each thing you
implement.

> This sort of goes for the packet stuff to; in
> theory it might be possible to "parse" packets.h and/or packets.c and
> generate the java code from it...

No, I don't think so. In best case you will be able to just generate
fields from .h definition - and I just use copy/paste for it (editing
char* to String etc). Most work is inside net routine - reading from
stream and it is very specific. You have to carefully read C source, to
detect all traps - some fields are packed, there are various lengths
fields, existence of some fields depend of values of other etc. I know
that it is unpleasant work, but I think that doing it by hand (with
possible occasional copy/paste from C) is only safe way to do this.



And now random ramblings about the changes that would need to be done to
the client.


Two biggest problems are network format change and tiles change. Network
format change is quite straighforward - and of course a lot of work.
Tiles are worse problem. For now all tiles are handled by Client in
quite specific way - they are kept on disk in cache, and all images are
referenced indirectly by SoftReference to ImageIcon. SoftReference
allows for automatic gc of unused images (they are major factor for
memory usage) and ImageIcon instead of direct Image allows to change
tileset/scale at the fly without going through all points where tile is
used. Unfortunately at 1.8.1 all offsets for images were hardcoded and I
followed the way. Now with tilespec advent handling of tile request
would have to change.

My idea to do it would be to parse tilespec and put all splitted images
in cache as before - by instead of naming them by tileset image name +
offset, name them by tilespec name (AFAIK tilespec names are unique
between particular files, so it should not be a problem). Then instead
of doing 

public FlashingIcon getUnitIcon(int gid)
{
        return getIcon(unitIcons,"units",gid);
}

there would be just a one big

public FlashingIcon getIcon(String name)
{
        return tileManager.lookupIconInSomeHashtable(name);
}

Unfortunately TileManager could not be just created once for given
tileset and then scrapped during tileset change, as icons have to be
replaced inside ImageIcon to perform easy change of tileset/scale. So it
would be basically a singleton inside Client. It could be even shared by
few instances of Client (AFAIK,there is nothing  barring opening few
Clients in same JVM for playing on same or different servers).

Change of tile handling would also require some changes in CivMap - for
now it uses just integer variations for shadows/terrain neighbourhood.
This would have to be replaced by some stringbuffer mumbo-jumbo to get
"n0w0e1s0" format.

There is a lot of missing functionality in client - most notably
diplomacy (both interplayer and diplomat/spy unit) and starship
handling. Also the city/research dialogs are not finished. There is a
lot to do prettiness-wise - I was experimenting with semi-transparent
dialogs, but it is still unfinished and left in bad shape.

Another thing to consider is enabling applet support. It is impossible
with current way of handling images, but if cache could be set up on
server, it would be possible. Anyway, it would be very network intensive
- all images and classes would have to be downloaded each time -
possibly 2-3MB even with compression ?

To reassume, it would be great if you could continue to work on it - I
think that it has the potential, and for me both city/settler overlays
and nuke animation just beats gtk client by few lengths :) Tileset
scaling/change is also a bonus, but except that, there is not much for
now to compete with gtk version.

If you have any questions concerning why I've done something this way,
or how code works or is supposed to work :) feel free to ask. This list
is otherwise dead for a long time, so I think that we can discuss
everything on the forum - just in case somebody else would like to jump
at some point by reading archives.



Artur