exe replacement

a place to discuss it

Moderators: Deathifier, Sukayo

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Mon Feb 10, 2014 4:03 pm

FYI I decided to test standard EFS's handling of DAT files, and found that it's much more permissive than my code. With FARM.DAT, for example, you don't have to enter the full set of data. Any omitted data is set to a default: no resources harvested, in this case. And--unlike my code--it does look at the terrain/planet strings, so you can enter terrains and planets in any order you like. I've forced modders to stick exactly to the standard order.

If you like I'll do some more testing, to find out more exactly what's permitted, and you/we can rewrite our code to more closely match standard EFS. But I'm not sure it's worth the effort, as modders don't seem to have taken advantage of that extra flexibility.

flowerpot
Assault Legionnaire
Posts: 74
Joined: Mon Dec 17, 2012 8:54 am
Location: Nowhere, trying to conjure up a Symbiot AI
Contact:

Postby flowerpot » Mon Feb 10, 2014 10:38 pm

tichtich wrote:When you're ready, let me know if you think you're going to use my code. If so, I'll carry on for the time being, and I'll co-ordinate my work with you. There are a couple of things I'd like to discuss before continuing. Otherwise, I may do a bit more just for the sake of some more practice with Java, but I'll do it entirely on my own, for my own eyes only.


I will use your code. There are just a few things that need to be fixed.

Game.java

*HarvestResourcesAtCity: handle missing hexes near upper and lower edges of map ? Edit. This should be easy, just collect the resources from hexes to a temp var and then divide the temp var by the number of hexes from which you collected and then multiply by 19.

The next two have to do with my choice of stack structure: the units that are cargo are present only in the cargo list of their carrying unit, not in the hex stack. I hope this does not turn out to be a foolish choice.

*AddResourcesToHex: use StackIterator to iterate through stack, use Util.stackSize to get stack size

*CreateUnitInHex: use Util.stackSize to get stack size

Unit.java

*Unit: prev_owner needs to be -1 or owner ? spotted needs to be set, otherwise unit spotted only after it moves or someone else moves. Edit. simply running hex_proc.spotProc on the hex and stack will accomplish this.

FYI I decided to test standard EFS's handling of DAT files, and found that it's much more permissive than my code. With FARM.DAT, for example, you don't have to enter the full set of data. Any omitted data is set to a default: no resources harvested, in this case. And--unlike my code--it does look at the terrain/planet strings, so you can enter terrains and planets in any order you like. I've forced modders to stick exactly to the standard order.

If you like I'll do some more testing, to find out more exactly what's permitted, and you/we can rewrite our code to more closely match standard EFS. But I'm not sure it's worth the effort, as modders don't seem to have taken advantage of that extra flexibility.


Doesn't seem worth the effort if there aren't mods that use it.

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Tue Feb 11, 2014 4:42 pm

flowerpot wrote:*HarvestResourcesAtCity: handle missing hexes near upper and lower edges of map ?

That one's OK. I've used your own neighbours[] data, so my HexesWithinTwoOf method doesn't need any knowledge of the map structure. And I've tested it by putting cities close to the edge with the galaxy editor.

In fact, although my first idea was to use your neighbours[] data, I couldn't see where you were creating that data, and I assumed you hadn't implemented it. So I initially coded a more direct method that did need knowledge of the map structure. Consequently my initial tests did fail because of the missing hexes, and I corrected for it. Later I discovered where you were creating the neighbours[] data, so I recoded to use that. It's proabably less efficient that way, but the code is simpler, and it's more secure against any changes to the map structure.

That said, I've since realised that we need a more general method, one that gets hexes within any radius, since STRBUILD.DAT lets you specify the harvesting radius. (And EFS.INI lets you specify the radius of shields.) I'll add that to my TODO list.

flowerpot wrote:The next two have to do with my choice of stack structure: the units that are cargo are present only in the cargo list of their carrying unit, not in the hex stack. I hope this does not turn out to be a foolish choice.

*AddResourcesToHex: use StackIterator to iterate through stack, use Util.stackSize to get stack size

*CreateUnitInHex: use Util.stackSize to get stack size

I missed the cargo thing. Well, I noticed you were storing cargo that way, but I forgot about it when implementing those methods. I don't think there's anything wrong with your choice. Whichever choice you'd made would be well-suited to some situations and less well to others: sometimes you want to ignore cargo and other times you don't.

Perhaps a simpler alternative to StackIterator would be this (I haven't tried it):

Code: Select all

public Stack getExpandedStack(Stack stack) {    // Returns a temporary stack with cargo listed separately
    Stack ret_val = new Stack();
    for (Unit unit : stack) {
        ret_val.add(unit);
        for (Unit cargo : unit.cargo_list) {
            ret_val.add(cargo);
        }
    }
    return ret_val;
}


Then you can iterate through a stack with...

Code: Select all

for (Unit unit : getExpandedStack(stack)) {
    ...
}


flowerpot wrote:*Unit: prev_owner needs to be -1 or owner ? spotted needs to be set, otherwise unit spotted only after it moves or someone else moves.

Your own Unit constructor sets all the spotted values to false, but that doesn't stop me seeing other factions' units at the start of the game, before anyone's moved. Is that because of prev_owner? What is prev_owner? I had a feeling it might be to do with the regency offices (or whatever they're called), making it possible to record both the office to which the unit belongs and the player-faction that currently controls it. I didn't think it had anything to do with spotting.

Have you got any document or URL that gives an explanation of the contents of galaxy.gal? That would be useful to see.

A few other thoughts...

1. I've installed git, and read a little about it. But I'm not sure where to start. Should I start by creating a repository from your posted version (0.8.0)?

2. There were times I found myself needing to pass a hex as a parameter, and having to pass it as a triplet (p_idx, x, y). It would have been more convenient to pass the hex itself, but I couldn't do that because later (when I created a unit) I had to use the triplet, and there's currently no way to read the triplet from the hex. It might be worth adding p_idx as a field in Hex, and making all three fields in the triplet available with getters, e.g. hex.getX().

3. I'm confused about the way you handle the game object. Various methods outside Game need access to game data (including DAT tables). I see that HexProc and Battle store a reference to the game. I assume that's so you don't need to keep passing it as a parameter. (This will be important to me if I put all my economic stuff in Faction rather than Game.) HexProc initializes this reference in its constructor, while Battle does it with a method that is ultimately called by GUI (init). I can't see any reason for the different treatment.

I also see that in GUI, when loading a saved game, you use some setGame methods to overwrite the game reference stored by various GUI classes. If I understand correctly, this is because those references are pointing to the previously running game, not the newly loaded one. But why doesn't this apply to the game references stored by HexProc and Battle too? I appreciate that, unlike the GUI classes, HexProc and Battle belong to Game. But I don't see why that makes a difference. Loading works, but it seems to me it shouldn't!

Naively, it seems to me that this would be a lot simpler if you made a public static getter for GUI.game, so everyone could access it freely whenever they need it, instead of storing their own local copy.

4. Game.random is another variable that seems to be passed around and stored in various places. Why not have a getter for game.random, so that it can be got freely whenever it's needed (once you have game).

I appreciate you don't want to make public getters for everything. Some stuff should be passed as parameters, so you can see the flow of data. But these seem like cases where a public getter would make things much simpler. Storing local copies of references seems wrong to me.

But I'm new to Java and OOP, so what do I know? ;)

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Tue Feb 11, 2014 5:05 pm

P.S. When I suggested having a static getter for GUI.game, I meant that the GUI.game field would be static too. I'm assuming the program will only be handling one game at a time!

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Tue Feb 11, 2014 8:06 pm

flowerpot wrote:*HarvestResourcesAtCity: handle missing hexes near upper and lower edges of map ? Edit. This should be easy, just collect the resources from hexes to a temp var and then divide the temp var by the number of hexes from which you collected and then multiply by 19.


I didn't see your edit before I replied, and I misunderstood you. I thought when you said "missing hexes" you were referring to the fact that even-numbered columns have one less hex than odd-numbered columns. I understand now, and yes I'll do what you say. (If it was my own game design I wouldn't give people any credit for off-map hexes. I don't see any reason to encourage building right on the edge. But we're trying to be consistent with EFS rules.)

P.S. That said, if it's all right with you, I'll only give extra credit for ordinary terrain, not specials. It seems particularly wrong to give someone nearly double the gems from a gem hex, just because he built on the edge of the map. I don't know if standard EFS does that, but I did once notice that it seemed very generous to cities on the edge.

I'll also start compiling a list of the rules and algorithms we adopt where there's any difference from standard EFS, or there's any uncertainty about what standard EFS does.

flowerpot
Assault Legionnaire
Posts: 74
Joined: Mon Dec 17, 2012 8:54 am
Location: Nowhere, trying to conjure up a Symbiot AI
Contact:

Postby flowerpot » Tue Feb 11, 2014 8:43 pm

tichtich wrote:
flowerpot wrote:*HarvestResourcesAtCity: handle missing hexes near upper and lower edges of map ?

That one's OK.

Oh, sorry, I meant to compensate for the smaller number of harvestable hexes for cities near the upper and lower edges of the map. When the map ends there will be no more hexes for the city harvest. Compensation should be easy, just collect the resources from hexes to a temp var and then divide the temp var by the number of hexes from which you collected and then multiply by the maximum number of harvestable hexes per city. Edit. yes extra credit only for hexes not specials. I have seen 190 metals from mines near edges in standard EFS. Build five such mines on both edges of the map and you have 4 engineers per turn.

That said, I've since realised that we need a more general method, one that gets hexes within any radius, since STRBUILD.DAT lets you specify the harvesting radius. (And EFS.INI lets you specify the radius of shields.) I'll add that to my TODO list.


hexproc has algorithm that does that for radius r, but it's rather long. Perhaps the proper way is to use "Breadth first search" algorithm with maximum distance r from central hex as a parameter

flowerpot wrote:The next two have to do with my choice of stack structure: the units that are cargo are present only in the cargo list of their carrying unit, not in the hex stack. I hope this does not turn out to be a foolish choice.


I don't think there's anything wrong with your choice. Whichever choice you'd made would be well-suited to some situations and less well to others: sometimes you want to ignore cargo and other times you don't.


Well I was thinking its eg. computationally expensive to get stack size by counting all the items in the stack and things like this will bite me in the behind if and when it comes time code AI.

Perhaps a simpler alternative to StackIterator would be this (I haven't tried it):
<clip>


I would put a :facepalm: emoticon here but there isn't one. Why didn't I think of this ? Well I guess this is a case of two heads work better that one ...

flowerpot wrote:*Unit: prev_owner needs to be -1 or owner ? spotted needs to be set, otherwise unit spotted only after it moves or someone else moves.

Your own Unit constructor sets all the spotted values to false, but that doesn't stop me seeing other factions' units at the start of the game, before anyone's moved. Is that because of prev_owner? What is prev_owner? I had a feeling it might be to do with the regency offices (or whatever they're called), making it possible to record both the office to which the unit belongs and the player-faction that currently controls it. I didn't think it had anything to do with spotting.


I'm not 100% sure but I do think that prev_owner is precisely used for recording the true ownership of regency office units & cities. I think it goes so that "prev_owner" records the true owner of the units and "owner" is current owner or faction that currently controls those troops.

Regarding spotting the Game initVisibility() handles initial spotting and hex and planet map visibility.

Have you got any document or URL that gives an explanation of the contents of galaxy.gal? That would be useful to see.


I used the map editor sources from http://hyperion.twarriors.com/efseditor.html as a starting point for Phoenix. I sent you an email of earlier private version of Phoenix.galaxyreader which has comments from the galaxyeditor sources behind the variables of Unit and Structure. Beyond that I have no documentation of galaxy.gal other than cryptic hand written notes of the decoding of hex_buffer (planet maps) in Planet.java.

A few other thoughts...

1. I've installed git, and read a little about it. But I'm not sure where to start. Should I start by creating a repository from your posted version (0.8.0)?


In git bash (assuming you installed from http://git-scm.com/download/win) type

Code: Select all

git config --global core.autocrlf true
then check that its set with

Code: Select all

git config core.autocrlf

to properly handle line endings on windows. Further Git setup can be found here https://help.github.com/articles/set-up-git

The source files which were included in Phoenix_src_0.8.0.zip were actually meant as convenience for building the jar with a simple download. The proper git repository for Phoenix can be downloaded from github by typing

Code: Select all

git clone git://github.com/joulupunikki/Phoenix.git Phoenix

in git bash. And then you can open the Phoenix directory as a "File->Open project" in Netbeans and it has also some nice git integration.
2. There were times I found myself needing to pass a hex as a parameter, and having to pass it as a triplet (p_idx, x, y). It would have been more convenient to pass the hex itself, but I couldn't do that because later (when I created a unit) I had to use the triplet, and there's currently no way to read the triplet from the hex. It might be worth adding p_idx as a field in Hex, and making all three fields in the triplet available with getters, e.g. hex.getX().

This sounds sensible

3. I'm confused about the way you handle the game object. Various methods outside Game need access to game data (including DAT tables). I see that HexProc and Battle store a reference to the game. I assume that's so you don't need to keep passing it as a parameter. (This will be important to me if I put all my economic stuff in Faction rather than Game.) HexProc initializes this reference in its constructor, while Battle does it with a method that is ultimately called by GUI (init). I can't see any reason for the different treatment.


See (***) below ...

I also see that in GUI, when loading a saved game, you use some setGame methods to overwrite the game reference stored by various GUI classes. If I understand correctly, this is because those references are pointing to the previously running game, not the newly loaded one. But why doesn't this apply to the game references stored by HexProc and Battle too? I appreciate that, unlike the GUI classes, HexProc and Battle belong to Game. But I don't see why that makes a difference. Loading works, but it seems to me it shouldn't!




When in Gui.SaveWorker I say

Code: Select all

            try (FileOutputStream f = new FileOutputStream(save_name);
                    GZIPOutputStream gos = new GZIPOutputStream(f);
                    ObjectOutputStream s = new ObjectOutputStream(gos)) {

                s.writeObject(game);
                s.flush();


that writeObject will recursively "serialize" the entire game object with all members and members of members etc. From java docs: "To serialize an object means to convert its state to a byte stream so that the byte stream can be reverted back into a copy of the object." So since Battle and HexProc instances are sub-objects of game all the things (including references) in Battle and HexProc will be saved to a file. And can be restored with with a single readObject invocation.

Naively, it seems to me that this would be a lot simpler if you made a public static getter for GUI.game, so everyone could access it freely whenever they need it, instead of storing their own local copy.

4. Game.random is another variable that seems to be passed around and stored in various places. Why not have a getter for game.random, so that it can be got freely whenever it's needed (once you have game).

I appreciate you don't want to make public getters for everything. Some stuff should be passed as parameters, so you can see the flow of data. But these seem like cases where a public getter would make things much simpler. Storing local copies of references seems wrong to me.

But I'm new to Java and OOP, so what do I know? ;)


(***) All these things and the above different treatment of references of HexProc and Battle have a bit to do with my background as coder. I have a BA in comp. sci. specializing in algorithms so I'm sometimes clever (I hope) when writing complex algorithms but when it comes to software engineering I only have one lecture course and one student project under my belt. This is actually my first big software project. So many of the strange things you see are genuine newbie mistakes. For example when in Gui you see

Code: Select all

        pallette = Util.loadPallette("EFS.PAL");
        color_index = loadICM();
        unit_icons = Util.loadSquares(C.S_EFSUNIT_BIN, 92, 32 * 32);
        loadHexTiles();
        loadStructureTiles();

and then later you see

Code: Select all

    resources = new Resources(this);


and in resources you see

Code: Select all

    unit_spot = UnitSpot.readUnitSpot();
    ter_color = TerColor.readTerColor();
    public double[][][] getUnitSpot() {
         return unit_spot;
    }   
    public int[][] getTerColor() {
        return ter_color;
    }

you may rightly ask that what is he doing and the answer is when he started this project he was a (more) confused newbie and it didn't occur to him that it might be smart put the loading of all the, well, "resources" into a Resources class. And then you could do more and wrap that resources in a progress bar that says "please wait, loading game resources" instead of the program appearing to halt for a few seconds when it starts. And the reason you see different treatment is that at some point I got wise and started to put those things in their proper places and I have yet to refactor the wrongly written code since I didn't think it would confuse anybody cause nobody would bother to read my code much less start hacking on it :)

flowerpot
Assault Legionnaire
Posts: 74
Joined: Mon Dec 17, 2012 8:54 am
Location: Nowhere, trying to conjure up a Symbiot AI
Contact:

Postby flowerpot » Tue Feb 11, 2014 10:17 pm

tichtich wrote:P.S. When I suggested having a static getter for GUI.game, I meant that the GUI.game field would be static too. I'm assuming the program will only be handling one game at a time!


Well this is talking about vaporware but I have ideas for the AI where you have a copy of the game object which will run on an AI thread on a different core during the players turn (since most comps these days have at least two cores.) Thus increasing time used for AI computations while decreasing wait time between players turns. But this could easily be implemented as separate static ai_game field. Anyway this is just cheap talk and high fantasy at this point.

Edit: I released a new version and I started a "Phoenix announcements" thread on the "EFS Clone" subforum to announce new versions since we are flooding this thread with development messages.

flowerpot
Assault Legionnaire
Posts: 74
Joined: Mon Dec 17, 2012 8:54 am
Location: Nowhere, trying to conjure up a Symbiot AI
Contact:

Postby flowerpot » Wed Feb 12, 2014 3:39 pm

tichtich wrote:3. I'm confused about the way you handle the game object. Various methods outside Game need access to game data (including DAT tables). I see that HexProc and Battle store a reference to the game. I assume that's so you don't need to keep passing it as a parameter. (This will be important to me if I put all my economic stuff in Faction rather than Game.) HexProc initializes this reference in its constructor, while Battle does it with a method that is ultimately called by GUI (init). I can't see any reason for the different treatment.


HexProc is created in Game constructor. It is considered bad OO practice to pass self references in constructors because that could lead to situations where the object is referenced before it's construction is finished, specifically if unconstructed parts of the object are referenced that will lead to unpredictable results and bugs which can be hard to debug. So the way that Battle does it is correct I believe.


Naively, it seems to me that this would be a lot simpler if you made a public static getter for GUI.game, so everyone could access it freely whenever they need it, instead of storing their own local copy.


Well ... in retrospect I probably should have made Gui.game static in the first place. The problem now is if I remember my OO correctly instance members cannot be accessed from static context, so to have a static getter for game one would essentially have to introduce a new static variable Gui.game_static which would have to be always set in conjunction with Gui.game :/ Well that would not look good but would it still make sense if it simplified accessing game in the future ?

4. Game.random is another variable that seems to be passed around and stored in various places. Why not have a getter for game.random, so that it can be got freely whenever it's needed (once you have game).


That is remnant from times when contents of Battle were in Game and Battle was the only thing that used random. So yes, another of my oversights, random needs getter ...

I appreciate you don't want to make public getters for everything. Some stuff should be passed as parameters, so you can see the flow of data. But these seem like cases where a public getter would make things much simpler. Storing local copies of references seems wrong to me.


I took me a couple of months to notice that Netbeans refactoring facilities provide for auto generation of getter and setter methods :x and sadly they produce getVar_one type of names instead of getVarOne which I settled for.

But I'm new to Java and OOP, so what do I know? ;)


Being a (former) professional programmer you might intuitively know a lot about OOP. And you will likely pick up fast.

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Wed Feb 12, 2014 10:02 pm

flowerpot wrote:hexproc has algorithm that does that for radius r, but it's rather long. Perhaps the proper way is to use "Breadth first search" algorithm with maximum distance r from central hex as a parameter.


I can't easily see the algorithm in HexProc. In case you're interested, here's my original, more direct (and probably more efficient) method. It could be generalized to a variable radius. It doesn't use neighbours at all. It just does a calculation based on co-ordinates, and applies its own checks for wrap-around and map edge.

Code: Select all

    public List<Hex> getHexesWithinTwo(int p_idx, int x, int y) {    //RSW

        int n;
        int this_x, this_y;
        int first_y;

        List<Hex> ret_val = new LinkedList<>();
        Planet planet = planets.get(p_idx);    // Get planet from planet index

        for (int i = -2; i <= +2; i++) {    // For each of 5 columns of hexes centred on the given hex
            this_x = (x + i + C.PLANET_MAP_WIDTH) % C.PLANET_MAP_WIDTH;    // Column number, allowing for wrap-around and negative numbers
            n = 5 - Math.abs(i);    // Number of hexes to be got in this column (3-5)
            if (i == 0) {
                first_y = y - 2;
            } else if (i == 2 || i == -2) {
                first_y = y - 1;
            } else {    // Columns one away from given hex will be staggered relative to it, and depend on whether the column is odd- or even-numbered
                if (this_x % 2 == 0) {
                    first_y = y - 2;
                } else {
                    first_y = y - 1;   
                }
            }
            for (int j = 0; j < n; j++) {
                this_y = first_y + j;   
                if ((this_y >= 0) && (this_y < C.PLANET_MAP_COLUMNS)) {    // Check if within map. (C.PLANET_MAP_COLUMNS is number of hexes in column)
                    if (!((this_y == C.PLANET_MAP_COLUMNS - 1) && (this_x % 2 == 0))) {    //
                      ret_val.add(planet.planet_grid.getHex(this_x, this_y));    // Add the hex to the list
                    }
                }
            }
        }
        return ret_val;
    }


flowerpot wrote:Well I was thinking its eg. computationally expensive to get stack size by counting all the items in the stack and things like this will bite me in the behind if and when it comes time code AI.

Ah, I see. Well, it's probably not worth worrying too much about that now. It's difficult to predict in advance where the bottlenecks will be.

That said, one area where I've been thinking about efficiency is in the counting and consumption of resources. Since resources are stored in cargo pods, those procedures require searching through many units. One option is to introduce some secondary data structures, e.g. maintaining a list of cargo pods for each faction for each resource type. That's one of the reasons I've been very keen to encaspsulate all creating and destroying of units in a pair of methods, so that those lists can be updated whenever a cargo pod is created/destroyed. But it would be better to avoid such secondary data structures if possible, so I'll make do without them in the first place, and only add them if necessary. Without them, there seem to be two alternatives:
1. Search the entire unit list for relevant cargo pods.
2. Search every hex of the relevant planet. (With Universal Warehouse off, that means searching all 40 planets!)
I've noticed that EFS consumes resources starting from the top left of the map and working down to the bottom right. So I guess it's using option #2.

flowerpot wrote:I'm not 100% sure but I do think that prev_owner is precisely used for recording the true ownership of regency office units & cities.

Then I must have misunderstood you, because I thought you were connecting prev_owner to the spotting issue. Regardless of spotting, you're right that I should set prev_owner to -1 not 0 (if we're using -1 to indicate no previous owner).

flowerpot wrote:Regarding spotting the Game initVisibility() handles initial spotting and hex and planet map visibility.

Oh yes, I forgot about that. Still, automatically switching on all the spotted[] flags of a newly created unit doesn't seem the right thing to do. If you have a method for updating those flags for a single unit or stack, perhaps I should call that when I create the unit.

Thanks for sending me what info you have on the data in galaxy.gal. Some of the items remain unclear. Naturally, when you wrote your galaxy reader you didn't need to know exactly what all those values were used for. But as we start implementing more features, a few questions will arise.

Here's one that's already occurred to me. Like with camo, galaxy.gal has two fields for movement type, which you've called unit.move_type and type_data.move_type. I can't see any use for unit.move_type. However it seems to me that sometimes you are using one and sometimes the other. Since your Unit constructor always sets unit.move_type to FOOT, I think that probably means you're sometimes getting the wrong results. I suggest we delete unit.move_type and always access type_data.move_type.

flowerpot wrote:And then you can open the Phoenix directory as a "File->Open project" in Netbeans and it has also some nice git integration.

Thanks. I've done that, and I now have two projects called Phoenix, the repository version and my own version. I'm not sure what to do next. Should I manually edit all my changes into the repository version? Am I correct in thinking that the newly created Phoenix folder contains both the repository and a set of source files, so I can edit those source files, but no change is made to the repository until I commit?

flowerpot wrote:
It might be worth adding p_idx as a field in Hex, and making all three fields in the triplet available with getters, e.g. hex.getX().

This sounds sensible

Then I'll go ahead and do it.

flowerpot wrote:So since Battle and HexProc instances are sub-objects of game all the things (including references) in Battle and HexProc will be saved to a file. And can be restored with with a single readObject invocation.

I realised that those sub-objects were being saved and restored. My concern was that the restored references to game would point to the wrong place. But I realise now that makes no sense!

flowerpot wrote:(***) All these things and the above different treatment of references of HexProc and Battle have a bit to do with my background as coder. I have a BA in comp. sci. specializing in algorithms so I'm sometimes clever (I hope) when writing complex algorithms but when it comes to software engineering I only have one lecture course and one student project under my belt. This is actually my first big software project.

That's interesting. I assumed you had much more background in software development. My own degree (over 30 years ago!) was in Statistics and OR. I did hardly any programming at uni, but got a job at an OR-oriented software house shortly after graduating. I was supposed to be concentrating on OR, but I found myself more interested in the software development side of things, and ended up doing that for about 5 years. OOP was just appearing towards the end of that period, and I never used an OO language, though my approach was already vaguely OO.

I'll comment some more on passing "game" soon.

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Wed Feb 12, 2014 11:48 pm

flowerpot wrote:HexProc is created in Game constructor. It is considered bad OO practice to pass self references in constructors because that could lead to situations where the object is referenced before it's construction is finished, specifically if unconstructed parts of the object are referenced that will lead to unpredictable results and bugs which can be hard to debug. So the way that Battle does it is correct I believe.

Ah, I didn't think of that. But now I'm surprised the compiler doesn't object to Game's "new HexProc(this)", since it can see perfectly well that this is a case of passing a self-reference in a constructor.

I think the problem is only with passing a self-reference from a constructor, not to one, so I guess you could still get rid of Battleinit, put the initialisation in Battle's constructor instead, and call "new Battle" from Game.init. Nevertheless, the fact that we can't initialise in Game's constructor (and have to get Gui involved in calling init) is a minus for this system IMO.

flowerpot wrote:Well ... in retrospect I probably should have made Gui.game static in the first place. The problem now is if I remember my OO correctly instance members cannot be accessed from static context, so to have a static getter for game one would essentially have to introduce a new static variable Gui.game_static which would have to be always set in conjunction with Gui.game :/ Well that would not look good but would it still make sense if it simplified accessing game in the future ?

Yes, I had in mind a static variable, but there's no need to add an extra variable. Just add "static" to the existing declaration of game. I just tried it, and it seemed to work.

Still, it doesn't smell right. We would have sub-objects of Game going over its head to Gui to get access to its data! I'm sure purists wouldn't like it.

I've gone back to thinking about why HexProc and Battle are separate classes in the first place. They aren't natural objects. They don't have any data of their own. They provide units of functionality, but not of data. The same goes for the code I'm writing. I will eventually be adding some faction data, like money and tithe rate. But what I'm doing at the moment operates on shared data structures like the unit list. Splitting these methods off into separate classes makes sense from the point of view of functional modularity, but not from an OOP point of view. I think what we really want is to put some methods in a separate file, but not in a separate class. Java doesn't provide that option, so we're trying to find a way of splitting into classes with a minimum of overhead.

If we do stick with your current system, there are two variations. You've used a mixture of both.
1. Pass only game to the constructor, which stores it. The methods can then access specific fields via getters, e.g. game.getUnitTypes().
2. Pass specific fields as parameters to the constructor, and store them, e.g. with "this type_data = type_data". They can then can be accessed directly.
I quite like the second option. It has several advantages:
- If we use the same variable names as Game does, as in this example, the code will look the same whichever class it's in. This makes sense, given that we're only dividing into separate classes for the sake of having smaller files, and we can more easily cut and paste methods from one file to another.
- We don't need getters. (But do we need getters anyway? I see no strong reason not to make Game's data public or package-private.)
- If we don't pass game at all, then presumably we avoid the problem of passing a self-reference from a constructor. (Or is there a problem with passing references to one's own fields from a constructor?)
- We only have to look at the constructor's parameters to see what external data the class uses.
The down side is that every time we find we need to access another field in Game, we have to add to the constructor's parameter list, in both the declaration and the call to it.

Sorry I don't have a strong recommendation for how to proceed. I can't make up my own mind.

flowerpot
Assault Legionnaire
Posts: 74
Joined: Mon Dec 17, 2012 8:54 am
Location: Nowhere, trying to conjure up a Symbiot AI
Contact:

Postby flowerpot » Thu Feb 13, 2014 6:58 pm

I can't easily see the algorithm in HexProc.


The advantage with Breadth first search (and HexProc, what a name ...) is that they record the distance r of hex from a central hex, which is needed in eg. spotting calculations. And actually, that algorithm which you use in the modifications you sent to me can be modified into breadth first search with radius r as a parameter (assumes we add variable r to hex)

Code: Select all

    public Set<Hex> getHexesWithinRadiusOf(Hex hex, int radius) {   //RSW

        // Find the neighbours of the given hex, and then the neighbours of those neighbours.
        // To avoid adding the same hex more than once, use Set instead of List, as that leaves it to the RTE to avoid duplicates
        Set<Hex> ret_val = new HashSet<>();    // Set of hexes to be returned
        LinkedList<Hex> queue = new LinkedList<>();
        ret_val.add(hex);
        queue.add(hex);
        hex.r = 0;
        while (!queue.isEmpty()) {
            Hex father = queue.pop();
            Hex[] neighbours = father.getNeighbours();
            for (Hex child : neighbours) {
                if (child != null && ret_val.add(child)) {
                    child.r = father.r + 1;
                    if (child.r < radius) {
                        queue.add(child);
                    }
                }
            }
        }
        return ret_val;
    }


In case you're interested, here's my original, more direct (and probably more efficient) method. It could be generalized to a variable radius. It doesn't use neighbours at all. It just does a calculation based on co-ordinates, and applies its own checks for wrap-around and map edge.


Edit. Clever, heh, I dreamed up a very similar algorithm today but I used the neighbour lists instead.

Code: Select all

 if (!((this_y == C.PLANET_MAP_COLUMNS - 1) && (this_x % 2 == 0))) {    //


This reminds me to mention that the case of every second column missing one index at the lower end is a case of my lack of confidence in handling the unseen hexes in EFS when I did that part of Phoenix a year ago. The hex_buffer was difficult to reverse-engineer and since I couldn't see the lowest hexes in the planet display in EFS I felt that I cannot properly debug the map handling, so I decided to remove those hexes from Phoenix (of course I should have persisted and used the global map for debugging.) But it is troublesome because in some maps house factions have large stacks and even starting cities in those hexes and they will have to be relocated or discarded (as is currently done.) I intend to return to that part of the code some time and include the lowest unseen (but will be seen in Phoenix) hexes in Phoenix. So using the neighbor lists is safest way operate with the planet maps.


That said, one area where I've been thinking about efficiency is in the counting and consumption of resources. Since resources are stored in cargo pods, those procedures require searching through many units. One option is to introduce some secondary data structures,
clip
Without them, there seem to be two alternatives:
1. Search the entire unit list for relevant cargo pods.
2. Search every hex of the relevant planet. (With Universal Warehouse off, that means searching all 40 planets!)
I've noticed that EFS consumes resources starting from the top left of the map and working down to the bottom right. So I guess it's using option #2.


I considered this too, when building units at cities enough resources for building have to be found. This means iterating (and subtracting from and possibly deleting) through relevant resource pods until sufficient resources have been found. Currently unit building doesn't consider resources or technologies since those do not exist in the public repositories yet.

flowerpot wrote:Regarding spotting the Game initVisibility() handles initial spotting and hex and planet map visibility.

Oh yes, I forgot about that. Still, automatically switching on all the spotted[] flags of a newly created unit doesn't seem the right thing to do. If you have a method for updating those flags for a single unit or stack, perhaps I should call that when I create the unit.


HexProc.spotProc(Hex hex, List<Unit> stack) does this.
But as we start implementing more features, a few questions will arise.

Here's one that's already occurred to me. Like with camo, galaxy.gal has two fields for movement type, which you've called unit.move_type and type_data.move_type. I can't see any use for unit.move_type. However it seems to me that sometimes you are using one and sometimes the other. Since your Unit constructor always sets unit.move_type to FOOT, I think that probably means you're sometimes getting the wrong results. I suggest we delete unit.move_type and always access type_data.move_type.

Actually unit.move_type are set by Game.setMoveType() which is called in Game constructor. I'm wondering why Holistic included move_type in two places. Only logical explanation would be that unit.move_type was somehow computationally advantageous. Aargh, i checked usages for unit.move_type and unit_type.move_type and got 24 and 18 :( some refactoring if we delete unit.move_type.

flowerpot wrote:And then you can open the Phoenix directory as a "File->Open project" in Netbeans and it has also some nice git integration.

Thanks. I've done that, and I now have two projects called Phoenix, the repository version and my own version. I'm not sure what to do next. Should I manually edit all my changes into the repository version?

That shouldn't be necessary. (This collaboration with git is mostly new to me since I have so far been an individual standalone developer.) I'm thinking since the starting point of your mods were a snapshot of the Phoenix repository it should be possible to checkout that snapshot and then copy your modified files onto the checked out snapshot files and then commit them, then create a branch of that commit, and then merge that branch with the master branch. Actually I would be interested in trying this, if you could send me a latest version of your harvesting modifications.
Last edited by flowerpot on Thu Feb 13, 2014 7:26 pm, edited 2 times in total.

flowerpot
Assault Legionnaire
Posts: 74
Joined: Mon Dec 17, 2012 8:54 am
Location: Nowhere, trying to conjure up a Symbiot AI
Contact:

Postby flowerpot » Thu Feb 13, 2014 7:02 pm

(Note: I had to split the post into two since for some reason the forum software choked on my post as a whole)

Am I correct in thinking that the newly created Phoenix folder contains both the repository and a set of source files, so I can edit those source files, but no change is made to the repository until I commit?


True. Actually reading through the first chapter of http://git-scm.com/book might be very beneficial (it is not long) and if you have the time the second and third chapters (but these are longer) will cover basic git operation and branching. Also or alternatively in git bash type

Code: Select all

git help git
to see a html help file where you should see "gittutorial(7)" to get started, then see "Everyday Git" for a useful minimum set of commands.

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Fri Feb 14, 2014 6:40 am

flowerpot wrote:The advantage with Breadth first search (and HexProc, what a name ...) is that they record the distance r of hex from a central hex, which is needed in eg. spotting calculations. And actually, that algorithm which you use in the modifications you sent to me can be modified into breadth first search with radius r as a parameter (assumes we add variable r to hex)

Nice algorithm. I'll use it. But I'm reluctant to add r to Hex, as it's just for local temporary use. I can put r in a parallel queue instead:

Code: Select all

        public Set<Hex> getHexesWithinRadiusOf(Hex hex, int radius) {   //RSW
           
        Set<Hex> ret_val = new HashSet<>();    // Set of hexes to be returned
        LinkedList<Hex> queue = new LinkedList<>();
        List<Integer> queueR = new LinkedList<>();
       
        ret_val.add(hex);
        if (radius = 0) return ret_val;
       
        queue.add(hex);
        queueR.add(0);
        while (!queue.isEmpty()) {
            Hex father = queue.pop(); 
            int r = queueR.remove(0);
            Hex[] neighbours = father.getNeighbours();
            for (Hex child : neighbours) {
                if (child != null && ret_val.add(child)) {
                    int child_r = r + 1;
                    if (child_r < radius) {
                        queue.add(child);
                        queueR.add(child_r);
                    }
                }
            }
        }
        return ret_val;
    }

For some reason the IDE wouldn't accept a second pop, so I've used "remove(0)" instead. That seems to work. I also had to add a check for radius = 0, as the "child_r < radius" check isn't made at the start. I also tried a version with a new (local) class HexR having 2 fields, hex and r, making the queue a list HexR's. But the method here seems simpler.

flowerpot wrote:Currently unit building doesn't consider resources or technologies since those do not exist in the public repositories yet.

I suggest you leave the handling of resources up to me. I'll provide a general method that you can call. You could also call my method createUnitInHex for building the unit. Or I could call yours if you have one already.

flowerpot wrote:
If you have a method for updating those flags for a single unit or stack, perhaps I should call that when I create the unit.
HexProc.spotProc(Hex hex, List<Unit> stack) does this.

I'll use that, then. Do I need to initialise spotted[] before calling spotProc?

flowerpot wrote:I'm thinking since the starting point of your mods were a snapshot of the Phoenix repository it should be possible to checkout that snapshot and then copy your modified files onto the checked out snapshot files and then commit them, then create a branch of that commit, and then merge that branch with the master branch. Actually I would be interested in trying this, if you could send me a latest version of your harvesting modifications.

My version was based on 0.8.0, but I think the last version I downloaded was 0.8.3. Anyway I'll try merging soon, and see what happens. I've been putting it off until I've finished making the corrections we've discussed already.

flowerpot
Assault Legionnaire
Posts: 74
Joined: Mon Dec 17, 2012 8:54 am
Location: Nowhere, trying to conjure up a Symbiot AI
Contact:

Postby flowerpot » Fri Feb 14, 2014 10:35 am

tichtich wrote:Nice algorithm. I'll use it. But I'm reluctant to add r to Hex, as it's just for local temporary use. I can put r in a parallel queue instead:

I was thinking the algorithm would then have more general use with r recorded and returned as part of hex. But if there is no need for such an algorithm then I guess it isn't necessary.

Code: Select all

List<Integer> queueR = new LinkedList<>();
       

For some reason the IDE wouldn't accept a second pop, so I've used "remove(0)" instead. That seems to work.


Notice that you have declared queueR as List not LinkedList. The object which queueR refers to is LinkedList but queueR is of type List. So only List facilities (specifically not pop) will be available for queueR.

Code: Select all

int r = queueR.remove(0);
queueR.add(child_r);


Do these work ? I was under the impression that since List is of type Integer not int you need to use

Code: Select all

int r = queueR.remove(0).intValue();
queueR.add(new Integer(child_r));


flowerpot wrote:Currently unit building doesn't consider resources or technologies since those do not exist in the public repositories yet.

I suggest you leave the handling of resources up to me. I'll provide a general method that you can call. You could also call my method createUnitInHex for building the unit. Or I could call yours if you have one already.


I was thinking of you doing resources. Further considerations for resources are the display of resources in the planet window, calculation of resource amounts is needed for universal warehouse on and of.

I was thinking of doing research and technologies next and when unit building, resources and research and technologies are done then I would integrate them so that availability of resources and technologies is reflected in unit building. This would be grounds for a major release.

flowerpot wrote:HexProc.spotProc(Hex hex, List<Unit> stack) does this.

I'll use that, then. Do I need to initialise spotted[] before calling spotProc?

Yes. spotProc only sets spotted[] values for those factions which have succesfully spotted.

tichtich
Assault Legionnaire
Posts: 56
Joined: Thu Jan 26, 2006 2:01 am
Location: Bristol, UK

Postby tichtich » Fri Feb 14, 2014 2:43 pm

flowerpot wrote:I was thinking the algorithm would then have more general use with r recorded and returned as part of hex. But if there is no need for such an algorithm then I guess it isn't necessary.

My thinking is that r is not a property of a single hex; it's a property of a pair of hexes (the distance between them). So it would make no sense to store it as a property of individual map hexes. It's meaningful to temporarily associate it with a single hex in the context of this method because everything this method does is relative to the input hex. r is only a property of the queued hex in combination with the input hex. To make r a field in Hex is to imply that it's a property of a single hex. It's a convenient trick rather than a genuine reflection of what's being modelled. In effect, this method would be saying to Hex: "r has nothing to do with you, but please hold it for me temporarily, just for my convenience." If it wasn't clearly commented in Hex, a reader of class Hex would likely be very confused as to what r was doing there.

I think the most natural approach of all is the other one I mentioned, creating a new class that combines Hex and r:

Code: Select all

    private class HexR {    // Temporary object holding a hex together with its distance (r) from a given hex.
        Hex hex;
        int r;
        public HexR(Hex hex, int r) {
            this.hex = hex;
            this.r = r;
        }
    }
   
    public Set<Hex> getHexesWithinRadiusOf(Hex hex, int radius) {   //RSW

        Set<Hex> ret_val = new HashSet<>();    // Set of hexes to be returned
        LinkedList<HexR> queue = new LinkedList<>();
       
        ret_val.add(hex);
        queue.add(new HexR(hex, 0));
        while (!queue.isEmpty()) {
            HexR father = queue.pop();
            Hex[] neighbours = father.hex.getNeighbours();
            for (Hex neighbour : neighbours) {
                if (neighbour != null && ret_val.add(neighbour)) {
                    HexR child = new HexR(neighbour, father.r + 1);
                    if (child.r < radius) {
                        queue.add(child);
                    }
                }
            }
        }
        return ret_val;
    }

Although this is arguably more natural than my previous version, the association between hex and r was something purely local to this method, and so I preferred to hide it within the method. (I don't think you can declare an inner class within a method.)

I'll choose convenience over what's natural if the benefit is sufficient to outweigh the cost. So far I don't think it is, but if you have more uses in mind for r that might change. I think what you have in mind is that this method (or a similar one) could return the r's along with the hexes. That means the association between hex and r is being communicated outside the method, which makes my approaches less attractive. I would have to return a parallel list of r's as an extra parameter, or return a list of HexR's as the return value. The latter would mean putting class HexR in a separate file if I want this method to be callable from outside its class. (I'm not sure I like the fact that Java requires the creation of a new class any time you want to associate two objects, combined with the fact that public classes must be in a separate file. As far as I dimly recall, C's shared structs don't have to be each in a separate file.)

This is rather a long discussion for such a minor point, but for me this is a learning exercise, so I like to consider the different ways of doing things.

BTW I consider this method a general utility, and expect it to be called from at least one other place. I was thinking of putting it in util/Util. (Or maybe wherever you keep your method for calculating the distance between two hexes, as the two methods seem related.)

flowerpot wrote:Notice that you have declared queueR as List not LinkedList. The object which queueR refers to is LinkedList but queueR is of type List. So only List facilities (specifically not pop) will be available for queueR.

Good point. I wasn't paying enough attention to that distinction.

flowerpot wrote:Do these work ? I was under the impression that since List is of type Integer not int you need to use

Code: Select all

int r = queueR.remove(0).intValue();
queueR.add(new Integer(child_r));

They seem to work. I guess the compiler is intelligent enough to translate for me. But your way seems safer.

flowerpot wrote:I was thinking of you doing resources. Further considerations for resources are the display of resources in the planet window, calculation of resource amounts is needed for universal warehouse on and of.

I've already refactored to create a separate calculateHarvest method, because the city display tells you how much the city is producing. The individual resource display (right-click on a resource in the planet window) tells you your total production on the planet and on all planets, as well as consumption totals, so I'll provide methods for those too. There seems to be no end to it! ;)

I'm still thinking about how to divide my methods into files, and what to do about passing game data between them. I'll leave them all in Game for now.


Return to “Fixing it ourself”

Who is online

Users browsing this forum: No registered users and 14 guests