Fix: Incorrect error messages on placing water on scenario editor #9560
+2
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SamuXarick
force-pushed
the
Fix-Incorrect-error-messages-on-placing-water-on-scenario-editor
branch
2 times, most recently
from
ac07e44 to
ca082ad
Compare
|
It is ready to review now. |
TrueBrain
reviewed
Sep 18, 2021
SamuXarick
force-pushed
the
Fix-Incorrect-error-messages-on-placing-water-on-scenario-editor
branch
from
ca082ad to
008e9ac
Compare
TrueBrain
reviewed
Sep 19, 2021
src/water_cmd.cpp
Outdated
| /* can't make water of water! */ | ||
| if (water && (!IsTileOwner(current_tile, OWNER_WATER) || wc == WATER_CLASS_SEA)) continue; | ||
| /* Allow to make water of water, except in these specific situations. | ||
| * The checks done via CMD_LANDSCAPE_CLEAR can provide better error messages if any. */ |
There was a problem hiding this comment.
This comment doesn't really help the reader. "These specific situations" is what is important. As the code is not obvious in what those situations are.
Also, the "make water of water" doesn't make sense anymore.
You have to look at it this way: if you read this in 1 year, would you still understand what this code does? And if not, will the comment help you understanding?
So what are those specific situations? Well, if I understood you correctly, your intentions are:
- Building a canal on your own canal should be skipped.
- Building a canal on a
OWNER_NONEowned canal should be skipped.
So the comment should at least mention that.
Basically, what you made this if do now, is something like:
Prevent building a canal over your own or OWNER_NONE owned canal.
src/water_cmd.cpp
Outdated
| if (water && (!IsTileOwner(current_tile, OWNER_WATER) || wc == WATER_CLASS_SEA)) continue; | ||
| /* Allow to make water of water, except in these specific situations. | ||
| * The checks done via CMD_LANDSCAPE_CLEAR can provide better error messages if any. */ | ||
| if (water && IsCanal(current_tile) && Company::IsValidID(_current_company) && (IsTileOwner(current_tile, _current_company) || GetTileOwner(current_tile) == OWNER_NONE)) continue; |
There was a problem hiding this comment.
There are two things in this line that are weird to me.
- The
IsValidIDis not obvious why it is there. So either you have to put it in the comment why, but a few lines above they use a more verbose way:_game_mode != GM_EDITOR. - For one check you use
IsTileOwner, for the nextGetTileOwner. Why? Is there something wrong with usingIsTileOwnertwice? (hint: there isn't, asIsTileOwnerexecutesGetTileOwner)
Additionally changes the behaviour of placing sea on sea/river/canal and placing canal/river on canal to (over)build, instead of disallowing it
SamuXarick
force-pushed
the
Fix-Incorrect-error-messages-on-placing-water-on-scenario-editor
branch
from
008e9ac to
c42f328
Compare
TrueBrain
approved these changes
Sep 19, 2021
SamuXarick
deleted the
Fix-Incorrect-error-messages-on-placing-water-on-scenario-editor
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Problem
I found some issues when placing sea/canal/river on scenario, the error messages don't always make sense and the behaviour is a little inconsistent when compared with each other. Listed below is the behaviour tested on 12.0-beta2.
On the scenario editor...
Building sea on sea: "Can't build canals here... already built"
Building sea on canal (owner none): "Can't build canals here... already built"
Building sea on canal (company owned): "Can't build canals here... already built"
Building sea on river: "Can't build canals here... already built"
Building canal on sea: builds with no error
Building canal on canal (owner none): "Can't build canals here... already built"
Building canal on canal (company owned): "Can't build canals here... already built"
Building canal on river: builds with no error
Building river on sea: builds with no error
Building river on canal (owner none): "Can't place rivers here... already built"
Building river on canal (company owned): "Can't place rivers here... already built"
Building river on river: overbuilds with no error
On a game...
Building canal on sea: builds with no error, cost £3,750
Building canal on canal (owner none): "Can't build canals here... already built"
Building canal on canal (self-owned): "Can't build canals here... already built"
Building canal on canal (other company): "Can't build canals here... already built"
Building canal on river: builds with no error, cost £3,750
Description
Below is the behaviour after the PR, which seem a bit better.
On the scenario editor...
Building sea on sea: overbuilds with no error
Building sea on canal (owner none): builds with no error
Building sea on canal (company owned): "Can't build canals here... owned by 'company'"
Building sea on river: builds with no error
Building canal on sea: builds with no error
Building canal on canal (owner none): overbuilds with no error
Building canal on canal (company owned): "Can't build canals here... owned by 'company'"
Building canal on river: builds with no error
Building river on sea: builds with no error
Building river on canal (owner none): builds with no error
Building river on canal (company owned): "Can't place rivers here... owned by 'company'"
Building river on river: overbuilds with no error
On a game...
Building canal on sea: builds with no error, cost £3,750
Building canal on canal (owner none): "Can't build canals here... already built"
Building canal on canal (self-owned): "Can't build canals here... already built"
Building canal on canal (other company): "Can't build canals here... owned by 'company'"
Building canal on river: builds with no error, cost £3,750
Limitations
Still incorrect error message for 'Building sea on canal (company owned)'
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.