Basic multiturn irrigation #555
Conversation
I wouldn't worry too much about multiplayer yet, just single player to start seems very reasonable. Overall I think this looks good! One other code pointer, I think we can set the button |
C7GameData/Tile.cs
Outdated
| @@ -251,6 +251,7 @@ public static Tuple<int, int> NeighborCoordinate(int X, int Y, TileDirection dir | |||
| } | |||
| return Tuple.Create(X, Y); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
could you revert this extra line?
C7Engine/EntryPoints/TurnHandling.cs
Outdated
| @@ -69,6 +70,7 @@ internal static void AdvanceTurn() { | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
could you revert this extra line?
| if (currentWorkerJob.Equals(unit.WorkerJob)) { | ||
| totalProgress += unit.WorkerProgressTowardsJob; | ||
| } else { | ||
| // reset Unit working on other jobs |
There was a problem hiding this comment.
This needs to be reset if the worker is selected/moved while doing the work - otherwise you can start irrigating on a legal tile, then move to a different tile and let the irrigation finish.
Here I managed to irrigate a hill
There was a problem hiding this comment.
Correct, I knew that but forgot to add under things not yet working.
C7Engine/TileExtensions.cs
Outdated
| if (currentWorkerJob == null) { | ||
| Log.Error($"can't call method FinishWorkerJob without WorkerJob"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'd leave out this null check - there's already a null check in the caller.
C7Engine/TileExtensions.cs
Outdated
| continue; | ||
| } | ||
|
|
||
| if (currentWorkerJob.Equals(unit.WorkerJob)) { |
There was a problem hiding this comment.
Is there a reason to use .Equals instead of ==? (Genuine question, I'm not super well versed in the nuances of C#)
There was a problem hiding this comment.
Okay I checked, you are right these are just my habits from other programming languages.
C7Engine/TileExtensions.cs
Outdated
| if (currentWorkerJob == null) { | ||
| Log.Error($"can't call method FinishWorkerJob without WorkerJob"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Similarly I'm not sure this null check is necessary
There was a problem hiding this comment.
The method is designed to be reusable, which is why I added the line. Right now a nullpointer would be thrown when I compare with a nullvalue, but that won't be the case anymore once I use ==. But still if somebody would call the function with null it would behave rather unintuitive and might create hard to find bugs which is why I thought it better to add the line. If we are concerned Efficiency/Performance I can also take it back out.
There was a problem hiding this comment.
I don't think it's a question of efficiency or performance, but more a question of why this method is special compared to all other methods - most other methods don't have explicit checks that the arguments are non-null.
You could probably get away with a comment like // REQUIRED: currentWorkerJob != null
Removed not needed null checks, Used == instead of Equeals for string Comp added a few more comments.
@TomWerner
I have created a first version and am looking for a bit of feedback before diving deeper in,
What it does for now:
What it does not for now:
What I am not sure about:
I update the progress on the end of the current turn (Which seems to fit with the functionality described in the doc). But you handle other similar things (healing) in OnBeginTurn.
How would I even begin to make a different behavior Multiplayer (update each Worker individually) vs. Singleplayer (Update all workers at one point and then check if the Job is done)