Skip to content
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

Healing: Do heal on turn 1. #3562

Merged
merged 6 commits into from Oct 21, 2018

Conversation

@jostephd
Copy link
Member

commented Sep 16, 2018

Discussed on the forums: https://r.wesnoth.org/t48817

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

That thread sounds kinda controversial; I wonder if this should be made an option in the scenario WML?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

i currently think it should not heal on the first turn of the the first player only, so that the scenario designer can place 'wounded' units at start, it also make somehow sense as there is side turn start but not really a side turn 'transition' for the first turn of the first player.

jostephd added a commit to jostephd/wesnoth that referenced this pull request Sep 22, 2018
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2018

@gfgtdf I agree. It would be cleaner not to do healing on side 1 turn 1 and let scenario authors do that by themselves if they want. I pushed the according patch. However, there is an oddity: on master current_side() returns 1 both for player turns and for AI turns. If I backport the patch to 1.14 (just for testing. I know it can't be committed to a released branch) then current_side() correctly returns 1 and 2 alternately.

@jostephd jostephd self-assigned this Sep 26, 2018

@shikadiqueen shikadiqueen force-pushed the wesnoth:master branch from 71a6daf to 60fd504 Oct 14, 2018

@jostephd jostephd force-pushed the jostephd:heal-turn-1 branch from 65c26f8 to c04ccb4 Oct 15, 2018

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 15, 2018
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2018

Rebased to new master

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

Check that the side number is correct, per your previous comment,
Then Squash, Rebase and Merge.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

I wonder whether these checks are sufficient, meaning if for example UMC change the initial turn number (eg change the turn numbesr in a prestart event), how should the code behave.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

The old code said "if turn > 1" and we're changing it to read "if turn > 1 or side > 1".

So if they played with turn numbers, this is a non-issue.

The only issue I could see if is they (effectively) don't have a "side 1", or they depended upon no healing occurring for any side on turn 1.

If they're playing games with turn and/or side numbering, there's not a lot we can do about it.

I imagine, in the rare case where they actually depend upon no healing at all throughout all of turn 1, well, a change log entry (which this PR is missing, BTW), should warn them. But there's not a lot we can do about that.


ETA: Well there might be problems if we allow turn 0 or turn -23, but we'd have had those problems, anyway. It would be nice if "turn" were "unsigned int" with the first turn being turn "0" and displaying as "1", but that's entirely another issue.

@GregoryLundberg GregoryLundberg added this to the 1.15.0 milestone Oct 18, 2018

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

The old code said

well even if the odl code also bheved liek that, this is not really a a reason, if we are already working opn this we shodul aim for a perfect/thought_though solution becasue we usually don't want to change things like this twice (in particular since it might be a compatability break)

The only issue I could see if is they (effectively) don't have a "side 1"

this is probably a more realisitc scenario, anyone with an opinion on how to handle this case?

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

The only solution I can see is to have a internal side number (index into array?) and a mapping from external side to that index.

What we mean is: "Regardless of the side number you assign, the first side to move on the first turn does not get a healing phase. But, thereafter, healing phases occur for all sides and turns."

Or we could just set a (default: false) doHealingPhase which is set true at the end of every movement phase.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

Or we could just set a (default: false) doHealingPhase which is set true at the end of every movement phase.

I think this is the best solution, it also has the advantage that it allows umc to easily enable the old behviour of they want.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

Agreed

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Does this "no side 1" block merging, or is it a separate future enhancement? About doHealingPass, I assume it'd be initialized to false, then the code would do if (doHealingPass_) calculate_healing(); doHealingPass_ = true;, is that right? I don't understand how adding an internal variable would help UMC.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

I think the idea is, while this could merge as-is, it would be better to take a few minutes and change the side/turn test(s) into a test against a boolean which is set to false when starting a scene and set true at the end of every side/turn.

I guess what he's suggesting is that there be some option to revert back to the old behavior. I would agree if this goes into 1.14, but don't think it's needed if this goes into master (1.15) only (which is the current milestone).

Oh, and, yes, what you're proposing is what I had in mind. Just don't forget to initialize to false before any turns in every scenario

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

I don't understand how adding an internal variable would help UMC.

my idea way to make the doHealingPass_ variable accessible via lua/wml like wesnoth.game_config.do_healing_phase = true/false

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

I'm not sure what I think of the idea that if there's no side 1, then the first side on turn 1 shouldn't receive healing. Is there currently any special significance to side=1? That is, if I took a scenario that has a side=1 and incremented all side numbers, and updated all references to them (like side N turn M event names), would there be any visible change?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

well yes, the colors ans side numbers in the sttus panels would be different. also side numbers have to be in order so when you want to do that you still have to put a side with side=1 at the top of the file with controller=null (disabled side)

@jostephd jostephd force-pushed the jostephd:heal-turn-1 branch from c04ccb4 to c7cda90 Oct 20, 2018

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 20, 2018
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Rebased, converted side/turn number check to boolean, confirmed side/turn numbers are right and healing happens on side x turn y whenever x>1 or y>1 (in a scenario that did have side 1).

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

you also have to read/write do_healing_ when loading scenarios/saves, otherwise it will be reset to 0 when a game is reloaded.

@jostephd jostephd force-pushed the jostephd:heal-turn-1 branch from c269b56 to 03f0ee1 Oct 20, 2018

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Moved the logic to game_state as discussed on IRC.

Right now it's initialized to false in the game_state ctor and set to true in play_controller. This feels like the logic is split between two classes.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Code for testing
diff --git a/data/campaigns/The_Hammer_of_Thursagan/scenarios/01_At_the_East_Gate.cfg b/data/campaigns/The_Hammer_of_Thursagan/scenarios/01_At_the_East_Gate.cfg
index a5aac138328..eb4973bd075 100644
--- a/data/campaigns/The_Hammer_of_Thursagan/scenarios/01_At_the_East_Gate.cfg
+++ b/data/campaigns/The_Hammer_of_Thursagan/scenarios/01_At_the_East_Gate.cfg
@@ -58,7 +58,9 @@
             [modifications]
                 {TRAIT_LOYAL}
             [/modifications]
+            hitpoints=1
         [/unit]
+        {UNIT 1 (White Mage) 1 14 ()}
     [/side]
     # wmllint: validate-on
 
@@ -82,6 +84,8 @@
             grouping=offensive
             attack_depth=5
         [/ai]
+        {UNIT 2 (Elvish Shyde) 6 14 (max_moves=0)}
+        {UNIT 2 (Wolf Rider)   6 15 (max_moves,hitpoints=0,1)}
     [/side]
 
     [label]
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Added a test. It reveals that rest healing isn't done on turn 2, which looks like a bug. It also reveals that rest healing is done even if the units moved which is definitely a bug.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Added a test. It reveals that rest healing isn't done on turn 2, which looks like a bug.

Fixed by the new push.

It also reveals that rest healing is done even if the units moved which is definitely a bug.

Test artifact. (The test unnecessarily used max_moves=0 to immobilize the units.)

@jostephd jostephd force-pushed the jostephd:heal-turn-1 branch from 4aa976e to 20b76b3 Oct 21, 2018

@jostephd jostephd merged commit 20b76b3 into wesnoth:master Oct 21, 2018

0 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

...what's this about not having a side 1? I didn't think that was allowed?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

...what's this about not having a side 1?

you can have a 'null' controlled side1 in which case the engine skips the turn of that side, making basicially side 2 the first side.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

Oh... would side 2 have had no healing phase in that event before this merge, then?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

no this merge only changes healing of the first turn (and fixes abug reated to rest healing). I think the commitmessaes are quite self explaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.