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

Add the worker "mine" action and fix UI updates after moves #499

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

TomWerner
Copy link
Contributor

@TomWerner TomWerner commented Jan 12, 2025

This PR has two parts, and I honestly would normally split them into two
(or even 3) PRs but git doesn't make this very easy (I miss my hg csplit)
and the GitHub UI doesn't seem to have good support for diffbases
(the diff from PR 498 shows up in this diff, despite this change being
on a separate branch; I know that it is part of the diff from Development,
but it would be nice to be able to easily review only the changes introduced
in this PR). Anyways, enough complaining.

The first part of this PR is straightforward: just mirroring all the existing logic
for building roads, but with method names for mines instead. Things like
the logic for whether we can build a mine, the keyboard shortcut, the
message to the engine, etc. This required adding logic for whether a
tile has a volcano, since that's a special tile for mines.

The second part was a bit trickier: ensuring that the UI was correctly
updated after moving a unit, when that unit still had movement points
available. This was partially implemented for keyboard-based moves in
Game.cs, but I suspect this was fundamentally racy - it raced the
message to the engine being received and updating the unit's position
against setting the selected unit. I tried duplicating this for the goto
logic and found it racy, so I suspect the same was true for the keyboard
moves, but I didn't exhaustively test this.

The solution here is to add a new message back to the UI from the
engine. Once the engine has finished doing all of its work on updating
the position of the unit we can safely reset the selected unit without
fear of races. This ensures that if a unit walks from a roaded tile
with a mine on it to a roaded tile that doesn't have a mine, the mine button
appears (for workers). It also fixes updating the move counter in the
lower right hand info panel.


Screenshots!

Before a mine is built:

Screenshot 2025-01-12 2 50 03 PM

After a mine is built:

Screenshot 2025-01-12 2 50 08 PM

#493

This PR has two parts, and I honestly would normally split them into two
(or even 3) PRs but git doesn't make this very easy (I miss my `hg csplit`).

The first part is straightforward: just mirroring all the existing logic
for building roads, but with method names for mines instead. Things like
the logic for whether we can build a mine, the keyboard shortcut, the
message to the engine, etc. This required adding logic for whether a
tile has a volcano, since that's a special tile for mines.

The second part was a bit trickier: ensuring that the UI was correctly
updated after moving a unit, when that unit still had movement points
available. This was partially implemented for keyboard-based moves in
`Game.cs`, but I suspect this was fundamentally racy - it raced the
message to the engine being received and updating the unit's position
against setting the selected unit. I tried duplicating this for the goto
logic and found it racy, so I suspect the same was true for the keyboard
moves, but I didn't exhaustively test this.

The solution here is to add a new message back to the UI from the
engine. Once the engine has finished doing all of its work on updating
the position of the unit we can safely reset the selected unit without
fear of races. This ensures that if a unit walks from a roaded tile
with a mine onto a roaded tile that doesn't have a mine, the mine button
appears (for workers). It also fixes updating the move counter in the
lower right hand info panel.
@TomWerner TomWerner force-pushed the twrner/worker-action-for-mines branch from 1fdaa49 to 03ad11b Compare January 17, 2025 04:13
@TomWerner TomWerner merged commit b7659af into Development Jan 17, 2025
3 checks passed
@TomWerner TomWerner deleted the twrner/worker-action-for-mines branch January 17, 2025 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants