some more AI code cleanup

[Cjkjvfnby]

I make mistake in prev commits. But nobody notice it because it was in dead branch.

upd. Small patch with production cleanup. It does not cover lot of function, to avoid intersections.

[Dilvish]

I make mistake in prev commits. But nobody notice it because it was in dead branch.

Hmm, that branch is a kind of fail-safe. Even if it currently is not exercised, I think it is prudent for it to remain. If you took it out then at the very least you'd have to add a comment warning about calling curBestMilShipRating() first, but I don't see any reason to go that route. I think we should simply correct the accidental name change (or conform the declaration and all other uses since that is the direction we want to go).

[Cjkjvfnby]

I make mistake in prev commits. But nobody notice it because it was in dead branch.

Hmm, that branch is a kind of fail-safe. Even if it currently is not exercised, I think it is prudent for it to remain. If you took it out then at the very least you'd have to add a comment warning about calling curBestMilShipRating() first, but I don't see any reason to go that route. I think we should simply correct the accidental name change (or conform the declaration and all other uses since that is the direction we want to go).

AI code is real maze. Often than I try to close deadends, you suggest don't touch it, lets someone else visit it later.

shipDesign.description is broken. It should be function not property. It is you choice to remove it of fix it.

shipDesign.partsInSlotType is not used

[Dilvish]

I moved these posts to a new thread so this conversation doesn't excessively clutter Moric's thread.

AI code is real maze. Often than I try to close deadends, you suggest don't touch it, lets someone else visit it later.

I'm not sure what other times you are referring to-- I suggest we deal with this issue here and if you think there is an overall problem in approach then you can start a clear discussion of that, but don't jumble it with the decision about this particular code. I explained why I considered those lines of code to be useful, for reliability/robustness purposes, even if they are not currently visited. Your words rather strongly imply you think this is a mistake, but you don't say why. Do you consider these lines a 'maze' or too complicated? Do you think I am wrong in considering them to have value? If so, why? Do you have anything substantive to say regarding this particular situation?

shipDesign.description is broken. It should be function not property. It is you choice to remove it of fix it.

Seems to be of no use to the AI, and unlikely to be of use for debugging. Any use not related to the AI is rather uncertain and distant, so I guess I'll remove it, it's trivial enough to recreate if there is a reason for it in the future.

shipDesign.partsInSlotType is not used

There are a great many parts of the interface that are not currently used but nevertheless should stay. More importantly however, this seems not likely to ever be of use to the AI, or at least I can't think of a potential use. Similar reasoning as above, & it's removed.

[Cjkjvfnby]

I moved these posts to a new thread so this conversation doesn't excessively clutter Moric's thread.

AI code is real maze. Often than I try to close deadends, you suggest don't touch it, lets someone else visit it later.

I'm not sure what other times you are referring to-- I suggest we deal with this issue here and if you think there is an overall problem in approach then you can start a clear discussion of that, but don't jumble it with the decision about this particular code. I explained why I considered those lines of code to be useful, for reliability/robustness purposes, even if they are not currently visited. Your words rather strongly imply you think this is a mistake, but you don't say why. Do you consider these lines a 'maze' or too complicated? Do you think I am wrong in considering them to have value? If so, why? Do you have anything substantive to say regarding this particular situation?

Whole code is maze. If I want to check single small function, I need inspect couple of modules. My attention stack overflows too fast.

reliability/robustness purposes. Code is very reliable I am scared to touch a little piece of logic because it can fail in any place across other modules. And sometimes it is not oblivious, because when something bad happens we pretend that all is ok.

Unusable code takes time:
- It takes time for reading
- It takes time then you try to understand how system works
- It take time for support

[Dilvish]

Whole code is maze. If I want to check single small function, I need inspect couple of modules. My attention stack overflows too fast.

I can sympathize-- when I first started working on the AI a framework was already in place but I only half understood it and there was no one to help explain it. I tried adding on to it in ways that made sense, and doing a little reorganization and refactoring, but not a whole lot of that. I acknowledge that my focus has been on augmenting AI capability, with less attention (some, but less) to making tha AI code as easy as possible for someone else to work with. Given that the AI is now tremendously more complex than when I started, it is probably even more difficult to dive into. I welcome the proposal you made for working on an overall reorganization of the AI code.

Code is very reliable I am scared to touch a little piece of logic because it can fail in any place across other modules.

This is starting to seem more like melodrama rather than an attempt at productive conversation.

And sometimes it is not oblivious, because when something bad happens we pretend that all is ok.

What are you talking about?

Unusable code takes time:
- It takes time for reading
- It takes time then you try to understand how system works
- It take time for support

Yes, yes, yes, I think you've restated this point quite a number of times now in multiple threads. I think continuing to repeat such generalities is a waste of time for everyone. Are you implicitly asserting the lines under discussion are unusable?

On a related but different point, I think there are actually a couple of unnecessary lines in curBestMilShipCost() and cur_best_mil_ship_rating() but they are different than the ones you are complaining about. I had written the initial lines

Code: Select all

    if (fo.currentTurn()+1) in bestMilRatingsHistory:
        bestMilRatingsHistory.clear()

before I understood very well how things went with loading an AI & starting new games, etc. I'm now pretty sure that these lines are unneeded and should come out.

[Dilvish]

I can't get the cleanup patch applied; rather large chunks get rejected and it doesn't appear to be a line-ending issue. Please try updating and remaking the patch & we'll see if it works then.

[Cjkjvfnby]

I can't get the cleanup patch applied; rather large chunks get rejected and it doesn't appear to be a line-ending issue. Please try updating and remaking the patch & we'll see if it works then.

I lunch [SVN 7944] with current trunk AI. Got some issues. Will inspect them now. In attached patch I change DEBUG -> ERROR

Melodrama Act I: "Life without designs."

Code: Select all

DEBUG AI : Error: exception triggered and caught:  Traceback (most recent call last):
DEBUG AI :   File "F:\projects\freeorion\FreeOrion\default\AI\ProductionAI.py", line 696, in generateProductionOrders
DEBUG AI :     try: addMarkDesigns()
DEBUG AI :   File "F:\projects\freeorion\FreeOrion\default\AI\ProductionAI.py", line 351, in addMarkDesigns
DEBUG AI :     print "Available Hulls: %s" % ([hull for hull in empire.availableShipHulls])
DEBUG AI : AttributeError: 'empire' object has no attribute 'availableShipHulls'

Link to patch on github: https://github.com/Cjkjvfnby/freeorion/ ... 25324.diff

[Dilvish]

I lunch [SVN 7944] with current trunk AI. Got some issues.

Yes, the error you posted was from the example lines I put near the start of addMarkDesigns() in ProductionAI.py, for Moric's reference, you can just comment them out until your executable gets updated with the next weekly build. The sdl branch just got merged back into trunk also, though, so you'll also have a problem with an updated line in AIState.py. You could temporarily change the reference at line 800 from design.attackStats back to design.directFireStats, but of course you'll need to undo that as soon as you get the new executable. There might be other trouble as well from the mismatch between your executable and the current content, although at first glance those are the only things I see.

[Dilvish]

Code: Select all

r7978 | dilvish-fo | 2015-02-23 16:55:15 -0800 (Mon, 23 Feb 2015) | 1 line
AI patch by Cjkjvfnby adding chat message to player if AI triggers an exception when trying to add ship designs

[Cjkjvfnby]

Possible attribute error, there is no such method AllEmpireIDs:
https://github.com/freeorion/freeorion/ ... get.py#L75

TargetType.TARGET_EMPIRE is not used assigned, do we need it?

[Geoff the Medio]

...there is no such method AllEmpireIDs

The A should not be capitalized...

Code: Select all

    def("allEmpireIDs",             AIInterface::AllEmpireIDs,      return_value_policy<return_by_value>());

[Cjkjvfnby]

...there is no such method AllEmpireIDs

The A should not be capitalized...

Code: Select all

    def("allEmpireIDs",             AIInterface::AllEmpireIDs,      return_value_policy<return_by_value>());

I have strong felling that this code is outdated, and can be removed. I am already remove big part of checking with TargetType.TARGET_SHIP

I think we can comment out this TargetType attributes:

Code: Select all

TARGET_INVALID = -1
TARGET_BUILDING = 0
TARGET_TECHNOLOGY = 1
TARGET_SHIP = 4
TARGET_EMPIRE = 6
TARGET_ALL_OTHER_EMPIRES = 7

We don't use it now, we can easily add them back then it will be really needed.

[Dilvish]

...there is no such method AllEmpireIDs

The A should not be capitalized...

Code: Select all

    def("allEmpireIDs",             AIInterface::AllEmpireIDs,      return_value_policy<return_by_value>());

I have strong felling that this code is outdated, and can be removed.

Are you saying that you think that AIInterface::AllEmpireIDs() is outdated? If so, please clarify why you think so, otherwise please clarify what you were referring to. Or did you simply hit the "quote" button by accident instead of "reply"?

I think we can comment out this TargetType attributes:...
We don't use it now, we can easily add them back then it will be really needed.

It seems to me that this kind of decision fits into the "toolkit" side of our work-- we're trying to provide a toolkit from which an effective AI can be built (by ourselves or others). I could understand some reasoning that things like "empire", "all other empires" and "technology" are a substantially different class of target than fleets, systems and planets, but from that toolkit perspective what would be the reasoning for have TargetType enum for fleets, systems and planets but not including buildings and ships?

[Cjkjvfnby]

Or did you simply hit the "quote" button by accident instead of "reply"?

Yes.

I think we can comment out this TargetType attributes:...
We don't use it now, we can easily add them back then it will be really needed.

It seems to me that this kind of decision fits into the "toolkit" side of our work-- we're trying to provide a toolkit from which an effective AI can be built (by ourselves or others). I could understand some reasoning that things like "empire", "all other empires" and "technology" are a substantially different class of target than fleets, systems and planets, but from that toolkit perspective what would be the reasoning for have TargetType enum for fleets, systems and planets but not including buildings and ships?

May be because AI can target fleet but can't target buildings and ships? Toolkit for not implemented features look strange.

upd.
Patch with remove checks for enum items that never set.
I comment out unused enum items.