Attached also his patch.
There are some style issues and such with the patch, but before looking those details, I'm going to put the current patch through some autogame testing. Those will take several days.
Reply To cazfi
There are some style issues and such with the patch, but before looking those details, I'm going to put the current patch through some autogame testing. Those will take several days.
Issues with other parts of the code interfered - so need to keep this on hold for another testing round.
Reply To cazfi
There are some style issues
Of course, you could already (re)read doc/CodingStyle and correct the issues you see yourself.
There's (still) something weird going on with master autogames, and I'd want to figure that out before pushing more stuff in (mostly because there's no meaningful way to test that it doesn't break things, when even the codebase to compare gives broken results)
Reply To cazfi
Of course, you could already (re)read doc/CodingStyle and correct the issues you see yourself.
Some quick notes:
- printf() used instead of log_..()
- Lots of trailing spaces
- Bad indentation (not 2, but 4)
- Variables declared in the middle of the block, and empty line separating declarations and code missing
- "||" in the end of the line, and not in the beginning
Couple of functional notes.
- Some of those values should be saved to the savegame, so e.g. counters won't start from zero after every save -> load cycle
- Maybe "city_list_size(giver->cities);" is a bit much of unconditional (AI level) omniscience about someone when we've just seen one of their units?
Run regression-test-like test (i.e. compared autogame with and without the patch) and there was no change on those autogames at all. So, even with this patch AIs had not traded any maps with each other.
1) It can be a coincidence that there was no single trade on those autogames, and in a typical autogame they would be trading maps (also: ruleset might make a huge difference here)
2) From a conservative PoV that we don't want AI behavior to change drastically (-> uncontrolled way) when we would not fully understand what would the consequences for it's abilities from that, having limited effect is a good thing
Reply To cazfi
Run regression-test-like test (i.e. compared autogame with and without the patch) and there was no change on those autogames at all. So, even with this patch AIs had not traded any maps with each other. 1) It can be a coincidence that there was no single trade on those autogames, and in a typical autogame they would be trading maps (also: ruleset might make a huge difference here)
No coincidence, AI actively performing (suggesting) map trade was not touched in this modification, which is to be seen as a start on the topic. So the benefit for now is restricted to human player activity. In that AI is still omniscient this doesn't make a difference for AI game-play. -- Free Willy
2) From a conservative PoV that we don't want AI behavior to change drastically (-> uncontrolled way) when we would not fully understand what would the consequences for it's abilities from that, having limited effect is a good thing
Reply To cazfi
Couple of functional notes.
- Some of those values should be saved to the savegame, so e.g. counters won't start from zero after every save -> load cycle
That's what I mentioned in my issue description (under "requirements"). I currently cannot do this, so perhaps somebody else can join in this function. Without it the modification doesn't make good sense.
- Maybe "city_list_size(giver->cities);" is a bit much of unconditional (AI level) omniscience about someone when we've just seen one of their units?
I don't think so as we haven't questioned omniscience of AI players so far. Furthermore, 50% of the worth calculation refers to player's own map value. -- Free Willy
Reply To (Anonymous)
Reply To cazfi
Couple of functional notes.
- Some of those values should be saved to the savegame, so e.g. counters won't start from zero after every save -> load cycle
That's what I mentioned in my issue description (under "requirements"). I currently cannot do this, so perhaps somebody else can join in this function. Without it the modification doesn't make good sense.
Savegame handling would go, e.g., to dai_player_save(), dai_player_load()
Opening the ticket on behalf of Free Willy. Earlier discussion on https://www.freelists.org/post/freeciv-dev/Diplomacy-Project-new-map-exchange-policy