New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revision of AI initial vs current meter usage #2009

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Morlic-fo
Contributor

Morlic-fo commented Feb 19, 2018

This PR is aimed to revise the AI usage of initialMeterValue vs currentMeterValue which I think wasn't sufficiently adressed by ac323f2.

For now, I only added couple of remarks and TODO comments. The todos must all be resolved before the PR is merged. The idea is to discuss and possibly resolve the relevant LOCs in this PR.

Throughout the AI code, I propose to use initialMeterValue by default and add explanatory comments if we are ever making use of currentMeterValue.

Additionally, I would strongly prefer to rename or at least to alias the interface functions to more accurately reflect their function in the AI code (something like thisTurnMeterValue and estimatedNextTurnMeterValue).

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Feb 19, 2018

Member

Throughout the AI code, I propose to use initialMeterValue by default and add explanatory comments if we are ever making use of currentMeterValue.

Sounds good to me; the default preference for changing to initialMeter absent an actual reason to use currentMeter is the viewpoint I took when doing my review, and requiring a written explanation for deviations seems helpful.

Additionally, I would strongly prefer to rename or at least to alias the interface functions to more accurately reflect their function in the AI code (something like thisTurnMeterValue and estimatedNextTurnMeterValue).

The aliases sound like a great idea. For now, we should just add these aliases in FreeOrionAI. The interface is shared with other scripting features as well, and it seems to me that it's possible we might wind up deciding to rename "CurrentMeter()" in the underlying C++ anyways. Changes to the actual interface can wait.

I'll plan to go through later and comment with my thinking on the particular various lines you've identified.

Member

Dilvish-fo commented Feb 19, 2018

Throughout the AI code, I propose to use initialMeterValue by default and add explanatory comments if we are ever making use of currentMeterValue.

Sounds good to me; the default preference for changing to initialMeter absent an actual reason to use currentMeter is the viewpoint I took when doing my review, and requiring a written explanation for deviations seems helpful.

Additionally, I would strongly prefer to rename or at least to alias the interface functions to more accurately reflect their function in the AI code (something like thisTurnMeterValue and estimatedNextTurnMeterValue).

The aliases sound like a great idea. For now, we should just add these aliases in FreeOrionAI. The interface is shared with other scripting features as well, and it seems to me that it's possible we might wind up deciding to rename "CurrentMeter()" in the underlying C++ anyways. Changes to the actual interface can wait.

I'll plan to go through later and comment with my thinking on the particular various lines you've identified.

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Feb 19, 2018

Contributor

The aliases sound like a great idea. For now, we should just add these aliases in FreeOrionAI. The interface is shared with other scripting features as well, and it seems to me that it's possible we might wind up deciding to rename "CurrentMeter()" in the underlying C++ anyways. Changes to the actual interface can wait.

Not sure how to implement this cleanly without manually editing / hackfixing the stub files.

For example, adding the following to patch_interface() and replacing the relevant function names throughout the code base works but is "incompatible" with the mock freeOrionAIInterface.py as far as IDEs are concerned.

    # rename current/initial meter value functions for the AI code
    # while leaving the interface function naming intact as they
    # are in shared interface with the server python wrapper.
    fo.universeObject.currentTurnMeterValue = fo.universeObject.initialMeterValue
    fo.universeObject.nextTurnMeterValueEstimate = fo.universeObject.currentMeterValue
    del fo.universeObject.initialMeterValue
    del fo.universeObject.currentMeterValue

The following alternative is ugly and does not forbid or warn about immediate function calls.

def next_turn_meter_value_estimate(universe_object, meter):
    return universe_object.currentMeterValue(meter)
Contributor

Morlic-fo commented Feb 19, 2018

The aliases sound like a great idea. For now, we should just add these aliases in FreeOrionAI. The interface is shared with other scripting features as well, and it seems to me that it's possible we might wind up deciding to rename "CurrentMeter()" in the underlying C++ anyways. Changes to the actual interface can wait.

Not sure how to implement this cleanly without manually editing / hackfixing the stub files.

For example, adding the following to patch_interface() and replacing the relevant function names throughout the code base works but is "incompatible" with the mock freeOrionAIInterface.py as far as IDEs are concerned.

    # rename current/initial meter value functions for the AI code
    # while leaving the interface function naming intact as they
    # are in shared interface with the server python wrapper.
    fo.universeObject.currentTurnMeterValue = fo.universeObject.initialMeterValue
    fo.universeObject.nextTurnMeterValueEstimate = fo.universeObject.currentMeterValue
    del fo.universeObject.initialMeterValue
    del fo.universeObject.currentMeterValue

The following alternative is ugly and does not forbid or warn about immediate function calls.

def next_turn_meter_value_estimate(universe_object, meter):
    return universe_object.currentMeterValue(meter)
@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Feb 20, 2018

Member

but is "incompatible" with the mock freeOrionAIInterface.py as far as IDEs are concerned

"incompatible" due to lack of typehinting for the new method names? Seems that should be easy enough to add to either the mock interface or to a stub file. Or was there something else?

Member

Dilvish-fo commented Feb 20, 2018

but is "incompatible" with the mock freeOrionAIInterface.py as far as IDEs are concerned

"incompatible" due to lack of typehinting for the new method names? Seems that should be easy enough to add to either the mock interface or to a stub file. Or was there something else?

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Feb 20, 2018

Contributor

Well yes, typehinting and autocomplete are not working, there are warnings for every usage of the new, unrecognized/unresolved function names and the old, nonexisting function names are still suggested for autocomplete. Nothing unexpected but very annoying.

We could certainly hackfix the mock file (or more so the generating file) to reflect the changes and that seems easy enough but it seems not very clean as it needs to be maintained. If there was a cleaner approach to handle this in one place, that would be preferable.

We could certainly hackfix the mock file (or more so the generating file) to reflect the changes and that seems easy enough but it seems not very clean as it needs to be maintained. If there was a cleaner approach to handle this in one place, that would be preferable.

Maybe we can simply run patch_interface() before the inspection? Haven't tested it yet. Maybe @Cjkjvfnby knows more/has a better idea?

Contributor

Morlic-fo commented Feb 20, 2018

Well yes, typehinting and autocomplete are not working, there are warnings for every usage of the new, unrecognized/unresolved function names and the old, nonexisting function names are still suggested for autocomplete. Nothing unexpected but very annoying.

We could certainly hackfix the mock file (or more so the generating file) to reflect the changes and that seems easy enough but it seems not very clean as it needs to be maintained. If there was a cleaner approach to handle this in one place, that would be preferable.

We could certainly hackfix the mock file (or more so the generating file) to reflect the changes and that seems easy enough but it seems not very clean as it needs to be maintained. If there was a cleaner approach to handle this in one place, that would be preferable.

Maybe we can simply run patch_interface() before the inspection? Haven't tested it yet. Maybe @Cjkjvfnby knows more/has a better idea?

Show outdated Hide outdated default/python/AI/AIFleetMission.py
Show outdated Hide outdated default/python/AI/AIFleetMission.py
Show outdated Hide outdated default/python/AI/AIstate.py
Show outdated Hide outdated default/python/AI/ColonisationAI.py
Show outdated Hide outdated default/python/AI/ColonisationAI.py
Show outdated Hide outdated default/python/AI/fleet_orders.py
@@ -365,6 +367,7 @@ def is_valid(self):
sys_partial_vis_turn = get_partial_visibility_turn(planet.systemID)
planet_partial_vis_turn = get_partial_visibility_turn(planet.id)
if (planet_partial_vis_turn == sys_partial_vis_turn and planet.unowned or
# TODO: Shouldn't this be initial meter value to be consistent with AIFleetMission checks?

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Feb 20, 2018

Member

I agree that the checks should be consistent (and perhaps a good place to refactor out some portion of the check to help maintain consistency), but on considering this more I think the better handling is currentMeter as here, not the change to initialMeter that I put into AIFleetMission. If the current planet population is expected to die off by next turn, then a colonization mission for it should be valid (though admittedly it is only in rather specific/rare circumstances I can imagine getting into that situation). If somehow the current pop is zero but the expected value for next turn is greater than zero (can't think of an actual scenario, but just if), then it does seem appropriate that the mission would not be valid.

@Dilvish-fo

Dilvish-fo Feb 20, 2018

Member

I agree that the checks should be consistent (and perhaps a good place to refactor out some portion of the check to help maintain consistency), but on considering this more I think the better handling is currentMeter as here, not the change to initialMeter that I put into AIFleetMission. If the current planet population is expected to die off by next turn, then a colonization mission for it should be valid (though admittedly it is only in rather specific/rare circumstances I can imagine getting into that situation). If somehow the current pop is zero but the expected value for next turn is greater than zero (can't think of an actual scenario, but just if), then it does seem appropriate that the mission would not be valid.

This comment has been minimized.

@Morlic-fo

Morlic-fo Mar 7, 2018

Contributor

I agree with your reasoning.

However, can we issue a colonization order if the initialMeterValue is non-zero?
If not, then we should probably leave this check as is but add an additional condition to can_issue_order() which previously would have been caught by is_valid().

@Morlic-fo

Morlic-fo Mar 7, 2018

Contributor

I agree with your reasoning.

However, can we issue a colonization order if the initialMeterValue is non-zero?
If not, then we should probably leave this check as is but add an additional condition to can_issue_order() which previously would have been caught by is_valid().

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Mar 7, 2018

Member

I believe that it could probably not issue the colonization order in that case (but I'll mention I also think there is no real harm I can think of in trying). I think it is already possible for the colonization order to be unexpectedly blocked from execution due to an armed invisible enemy, and it is my recollection that there is code in there somewhere that double checks if the colonization attempt actually 'took', and if not then it re-executes the order next turn. But at the moment I'm not recalling exactly where that is, and the way is_valid() is marking the order as issued seems odd to me (I think it always looks odd to me everytime I haven't seen it in a while but once I review more I wind up deciding to leave it as-is, which probably means that at the very least it should have a explanatory comment), and so if you'd like to review this handling more and possibly do some cleanup including adding the additional constraint you just proposed for can_issue_order(), that would be fine with me.

@Dilvish-fo

Dilvish-fo Mar 7, 2018

Member

I believe that it could probably not issue the colonization order in that case (but I'll mention I also think there is no real harm I can think of in trying). I think it is already possible for the colonization order to be unexpectedly blocked from execution due to an armed invisible enemy, and it is my recollection that there is code in there somewhere that double checks if the colonization attempt actually 'took', and if not then it re-executes the order next turn. But at the moment I'm not recalling exactly where that is, and the way is_valid() is marking the order as issued seems odd to me (I think it always looks odd to me everytime I haven't seen it in a while but once I review more I wind up deciding to leave it as-is, which probably means that at the very least it should have a explanatory comment), and so if you'd like to review this handling more and possibly do some cleanup including adding the additional constraint you just proposed for can_issue_order(), that would be fine with me.

@@ -401,6 +404,7 @@ def is_valid(self):
if not super(OrderInvade, self).is_valid():
return False
planet = self.target.get_object()
# TODO: Shouldn't this be initial meter value to be consistent with AIFleetMission checks?

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Feb 20, 2018

Member

I am not finding an Invasion-related population meter check in AIFleetMission for this to be inconsistent with, but regardless (and this is seeming to me like I made nearly the same comment re invasions above, but cannot find it now, though it is a bit related to the colonization check)-- if this check were only being done at the very last instant, just before issuing the actual invade order, then I'd agree that initialMeter is the way to go. Since I think it is often checked in advance, then it seems more logical to me to check currentMeter (though frankly, since this only matters for unowned planets, with current content I can't even see when those readings would give a different result),

@Dilvish-fo

Dilvish-fo Feb 20, 2018

Member

I am not finding an Invasion-related population meter check in AIFleetMission for this to be inconsistent with, but regardless (and this is seeming to me like I made nearly the same comment re invasions above, but cannot find it now, though it is a bit related to the colonization check)-- if this check were only being done at the very last instant, just before issuing the actual invade order, then I'd agree that initialMeter is the way to go. Since I think it is often checked in advance, then it seems more logical to me to check currentMeter (though frankly, since this only matters for unowned planets, with current content I can't even see when those readings would give a different result),

Show outdated Hide outdated default/python/AI/fleet_orders.py
Show outdated Hide outdated default/python/AI/turn_state.py
@Cjkjvfnby

This comment has been minimized.

Show comment
Hide comment
@Cjkjvfnby

Cjkjvfnby Feb 20, 2018

Contributor

Maybe we can simply run patch_interface() before the inspection? Haven't tested it yet. Maybe @Cjkjvfnby knows more/has a better idea?

No, patch_interface is patching module in runtime, I don't sure that PyCharm can recognize it.

You can just generate new mock or patch manually together with your changes. Feel free to commit changed mocks. Generator works in runtime, after patching.

Contributor

Cjkjvfnby commented Feb 20, 2018

Maybe we can simply run patch_interface() before the inspection? Haven't tested it yet. Maybe @Cjkjvfnby knows more/has a better idea?

No, patch_interface is patching module in runtime, I don't sure that PyCharm can recognize it.

You can just generate new mock or patch manually together with your changes. Feel free to commit changed mocks. Generator works in runtime, after patching.

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Feb 27, 2018

Contributor

Thank you for the thorough review. I could only skim your comments so far but what I saw made sense so far. I hope to find some time in the next few days to come back to this.

Contributor

Morlic-fo commented Feb 27, 2018

Thank you for the thorough review. I could only skim your comments so far but what I saw made sense so far. I hope to find some time in the next few days to come back to this.

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Mar 5, 2018

Contributor

Got to review and adress some of the points but still WIP.

Contributor

Morlic-fo commented Mar 5, 2018

Got to review and adress some of the points but still WIP.

@Cjkjvfnby

This comment has been minimized.

Show comment
Hide comment
@Cjkjvfnby

Cjkjvfnby Mar 9, 2018

Contributor

Suggestion about workflow:
This PR can be spitted to multiple small PRs, that can be merged separately from each other. Usually this speedups review process. You can create special branch/pr/issue in main/personal repo, that will contain whole feature overview.

PS. Please don't response this comment here, this thread have enough comments. Use https://github.com/orgs/freeorion/teams/ai-team

Contributor

Cjkjvfnby commented Mar 9, 2018

Suggestion about workflow:
This PR can be spitted to multiple small PRs, that can be merged separately from each other. Usually this speedups review process. You can create special branch/pr/issue in main/personal repo, that will contain whole feature overview.

PS. Please don't response this comment here, this thread have enough comments. Use https://github.com/orgs/freeorion/teams/ai-team

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Jul 31, 2018

Member

@Morlic-fo @Dilvish-fo Update / Status on this?

Member

geoffthemedio commented Jul 31, 2018

@Morlic-fo @Dilvish-fo Update / Status on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment