Preserve polygon order when loading game. #9963
Conversation
This is critical for "island" situations when one territory completely encloses another. By using hash map in this class, the "z order" of the territories is totally unpredictable. This affects our map because we use rail road zones (sea zones) inside land zones and we want to make sure they, and the units in them, are always selectable "above" the containing land territory.
|
I even tried a fairly sophisticated workaround. To avoid territories with "holes", I split the territory in half along the inner territory's centroid and "subtracted" the inner territory from the outer which is now defined by two polygon segments. However, this still had a thin black vertical line on the seam which doesn't work visually: |
Codecov Report
@@ Coverage Diff @@
## master #9963 +/- ##
============================================
- Coverage 27.16% 27.16% -0.01%
+ Complexity 7799 7798 -1
============================================
Files 1206 1206
Lines 79172 79172
Branches 10891 10891
============================================
- Hits 21511 21508 -3
- Misses 55588 55589 +1
- Partials 2073 2075 +2
Continue to review full report at Codecov.
|
|
I want to make you notice that, for example if you want to select a train and move it to "RR9", as well as in every other case you may need, TripleA already fully supports what you need: just (re)name "Tallin" as "Tallin Sea Zone", and so on. Side note, I would rather name "RR9" as "Tallin Hub", and so on: easier to make sense of history reports. |
|
@Cernelius no, I don't think so. Can you point me to where in the code that is enforced? I've observed that we're at the mercy of the string hashing algorithm which usually orders things the way you're describing, but is subject to change at any moment with new versions of Java |
|
This is a long standing issue. It all stems from the fact that TripleA uses the hack of looking at the zone name ending or starting as "Sea Zone" in order to draw it below every other zone which do not. However, the solution herein proposed looks like an other hack to me. How would map-makers be supposed to know that they have to make sure to have zones listed in a particular order in a file (being not the game file)? Surely you can document it, but it would be one more thing to check for manually, especially if the map-making utilities are not updated accordingly, featuring an automated listing discrimination. I'm not against this pull request, as it provides a solution, but I would rather suggest two alternative solutions. The best solution, in my opinion, would be allowing defining "holed" (Swiss cheese like) zones in polygons, so possibly to make sure never to have the same pixel belonging to more than one zone. Something related I would suggest is then having a map-making utility that gives you the full drawing of over-lapping areas, so you can check your polygons to make sure you aren't inadvertently having any number of them. If the best solution is not to be taken, I would rather suggest having a prompt asking you what zone you are interacting with every time you click on a pixel which belongs to two or more zones. In this case, when you click on a train is St. Petersburg and then click on the RR9, the program will ask you if you want to move the train to RR9 or to Tallin, but it should actually automatically select RR9 without giving you the prompt if the unit cannot move to Tallin (as it certainly cannot, since the unit is a sea unit and Tallin is a land zone). This feature would also allow intentionally having superimposed zones, opening some room to multidimensionality in map-making. Additionally, when you hover a pixel belonging to multiple zones, you should have as many "Territory" tabs available (changing the tab's name to be the zone's name). By the way, I believe that the correct English English spelling is "Tallinn", not "Tallin", but I don't know what was the correct spelling back in WW1. |
I know nothing of coding, so I cannot point you to anything there. If you don't think so, just test it and let me know if I'm wrong. All you have to do is renaming "Tallin" to "Tallin Sea Zone" and start the game, selecting a train and moving it to RR9. |
|
@Cernelius thanks for the feedback. So, my fix here isn't a hack like the current hack. There is nothing that is currently documented about the order in the polygons file, so this is a "hidden" change from the standpoint of map developers - nothing was promised about this before and nothing will be after (in coder lingo, we say that it's an implementation detail). In fact, it's just making things behave the way you would expect- if I list my territories as "Ter C, Ter A, Ter B" in order in the file, you would expect the game to load them in that order, not some random order. Yes, I wish we could allow "holed" polygons, but that's a huge change due to the way the I also like your prompt idea, but it would be another big change. I actually made the tool you suggest in a simple way- drawing polygons and centers with transparency so you can see overlaps and draw order/z order. I've attached an example that I was using when trying the workaround mentioned above. P.S. Thanks for the spelling tip- my friend is doing all the artwork, spelling, etc. I'm responsible for the technical parts. |
|
@jdimeo Assuming this request gets accepted, do you have any integrated plan for information? How is every map-maker (comprising those unable to read the codes of the program), now and forever in the future, supposed to know that zones preceeding other zones in the listing order of the I'm asking because the current "having-the-name-ending-in-Sea-Zone" appears to be esoteric enough that, apparently, not even a fairly experienced map-maker and developer like you knew about it, and this other process appears to me to be even more obscure. |
I see you have stumbled into the widespread problem of units in zones enclosing other zones (usually, sea zones with small islands within) drawing units in them inside the enclosed zones (sometimes you can see a battleship visibly grounded on an insland, or something like that). As a not-so-skilled mapmaker, my solution so far has been having a polygons, used specifically for the auto-placement only, in which the enclosing zones are line-cut the way you described. This is yet another actual problem that allowing "holed" polygons would solve, but which should be anyway solved one way or the other, or it will either keep plaguing newly made maps or obliging map-makers to find time-consuming ways around it (typically, re-making the placement manually). If you want to share that tool of you somewhere and explain it for non-developers, I'm sure other map-makers like me may be happy to use it while this problem is still actual (as I believe it has always been). As an example amongst several, you can simply start the popular "World War II v3 1941" and, on the first Germans turn, move the transport to "14 Sea Zone" and load 1 infantry and 1 armour into it: you will see 1 German armour on the island of Sicily and 1 Italian cruiser beached over the island of Crete, whereas they are both on the sea (on the other hand, the Italian armour then displayed over Sardinia is actually on land, instead: you just have no way, looking at the map only, to know whether a land unit you see on a small island is actually on land or on board of some ship in the surrounding sea zone). |
|
@Cernelius to answer your first question, the reason why I'm proposing this fix is because no every map maker is not supposed to know. It's replacing non-determinism (randomness) in the current game with determinism/predictability. It's not changing the game file syntax, or the documentation, or anything like that. Polygon order already affects the "z order" and which one is selectable or not. The problem is that in the current engine you have no control over it. As an advanced map maker, I was expecting to be able to reorder the You're right that this problem deserves a more comprehensive solution (the "Sea Zone" convention is also not well known/documented and doesn't work anyway since I have rail road zones and don't want to call them "X Sea Zone"), but that is outside the scope of this PR, which is a very lightweight change I hope to get included in the next release, precisely because it doesn't change anything for map making. @DanVanAtta are we able to get this merged? |
|
My map tools, which include a more advanced/optimal auto placement tool that doesn't put a box on top of any other territory's polys, are here: |
Wow. I'm looking forward at trying it, hopefully soon. |
But, isn't there also the risk that, for example, I (like you) make a map that relies on this ordering and, thereafter, like twenty years from now, a developer makes a related change making my map unplayable (by hiding a zone behind an other one) because he/she is not concerned about it as it is not a documented as an intentionally supported map-making process to define priorities when overlapping?
Actually, it is the land zones (not the sea (railway hub) zones) which have to be called ending or starting in "Sea Zone". The "Sea Zone" naming doesn't imply in any way that is a sea zone.
Yep. Just saying. |
| @@ -44,7 +44,7 @@ private PointFileReaderWriter() {} | |||
| public static Map<String, Point> readOneToOne(final Path input) throws IOException { | |||
| checkNotNull(input); | |||
|
|
|||
| final Map<String, Point> mapping = new HashMap<>(); | |||
| final Map<String, Point> mapping = new LinkedHashMap<>(); | |||
There was a problem hiding this comment.
Class javadoc should be updated to note that we preserve polygon ordering as read from the polygons.txt file.
That could still be a confusing comment and we'll need to make sure this class is only used to read polygon information. If the class is generic, the comment should be generic too I suppose. Regardless, a developer may wonder why we are not using LinkedHashMap
There was a problem hiding this comment.
I disagree, I think it's important that we don't publicize this change because it's not a fully adequate solution to the problem as @Cernelius pointed out. My goal here is just replacing the non-determinism of the hash map with the determinism of the linked hash map. I don't want map makers to depend on this behavior or make it a public API.
There was a problem hiding this comment.
I understand the desire to keep an unpublicized contract.
I do not think that is what I asked for. To get on the same page, my perspective is that javadocs are only read by developers, and only read while they are reading source code.
If order matters, one would expect a List to be used and not a Map. I can't speak to whether that is a worthwhile update, I would suspect no. Failing that though, even a quick line comment to a developer telling them why it would be a bug to switch this back to hashMap, would be helpful.
This is critical for "island" situations when one territory completely encloses another. By using hash map in this class, the "z order" of the territories is totally unpredictable. This affects our map because we use rail road zones (sea zones) inside land zones and we want to make sure they, and the units in them, are always selectable "above" the containing land territory.
Change Summary & Additional Notes
Release Note