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

Great War - two bugs in map #1331

Closed
Rogue77-NL opened this issue Nov 7, 2016 · 18 comments
Closed

Great War - two bugs in map #1331

Rogue77-NL opened this issue Nov 7, 2016 · 18 comments

Comments

@Rogue77-NL
Copy link

Hello,
The map Great War contains two bugs.

  • Land territory 'Corsica' is counted as sea zone 'SZ 51 Ligurian Sea'.
    This was already the case in previous versions, judging by the territory name shown in the bottom left corner of the game window, but I was able to move land units to 'Corsica' nevertheless. Since the latest update of the game to version 1.9 the latter is not possible anymore. The infantry unit present at 'Corsica' at the beginning of a game can still be selected.
  • Land territory 'Songea' and sea zone 'SZ 71 Mozambique Channel' are adjacent to eachother; this seems unintentional.
    This bug was too minor for me to report separately, but the Corsica thing is influencing gameplay more significantly, so if I'm submitting a report anyway... :)
    Thanks in advance for your time!
@ron-murhammer ron-murhammer added the Problem A problem, bug, defect - something to fix label Nov 16, 2016
@ron-murhammer ron-murhammer added Map Bug Backlog and removed Problem A problem, bug, defect - something to fix labels Nov 16, 2016
@simon33-2
Copy link
Contributor

simon33-2 commented Dec 30, 2016

The first of these issues requires clarification. I for one don't understand what is meant by the first issue. @Rogue77-NL

Personally, I'd be inclined to close it shortly if the raiser doesn't do so.

@NikitasKotsolakos
Copy link

NikitasKotsolakos commented Mar 26, 2017

For the second issue (Songea - SZ 71 connection), I submitted a pull request in the map repository.

For the first issue (Corsica territory problem), after investigation, I found that the game engine, when trying to get the territory that you hover, first tries to find a land territory else a water one. (MapData.getTerritoryAt() )
However, in order to determine if it is about a water territory or not, it uses:
Util.isTerritoryNameIndicatingWater() , which bases the decision on weather the territory name starts or ends with "Sea Zone".

For great_war map, this is not true, as Water territories start with SZ, so it leads to problem when the Sea Zone is checked before the Land zone (as it is in the case of corsica).

I do not know if this (start or end with Sea Zone) is supposed to be a requirement. All I could find is that in the TripleA Map Creator 1.0.1.4 release ( http://tripleadev.1671093.n2.nabble.com/TripleA-Map-Creator-1-0-1-4-Released-td4616118.html ) is mentioned:

Add more information to Part 1 for the Center Picker that tells the users to make any sea zones that contain islands end with "Sea Zone". (SDW)

So, if it is indeed a requirement, the great_war map should be updated. If it is not, the MapData.getTerritoryAt() should be updated to perform the check of weather the territory is water by some other means (probably get the territory by name, and then check isWater(), if territories are available there). In this case probably code should also be refactored so that Util.isTerritoryNameIndicatingWater is removed, in order to avoid similar problems in other places.
There is also the quick workaround to change Util.isTerritoryNameIndicatingWater in order to check also for starting or ending with SZ, but this doesn't sound like a good solution.

I am willing to provide the fix, but since I am rather new here, I am not sure which one is the correct one.

@Rogue77-NL
Copy link
Author

I guess the 1st issue doesn't need clarification after all, nor should the ticket be closed... ;)

@Cernelius
Copy link
Contributor

Cyprus is bugged, read made wrong too, as far as I know. Just it happens that there you have the territory you need superimposed, so the bug is not affecting the game.

It is the same thing that me and @veqryn corrected in New World Order (Baleares and Krym). However this map was actually updated by @veqryn, so I'm actually curious if I'm overlooking anything (is or was there another legitimate way beside the Sea Zone naming or simply avoiding overlapping, at the price of having at least a line from the sea zone border to the island?) or maybe he had plans to update the engine to support it otherwise, meanwhile being content at the map working?

@veqryn
Copy link
Member

veqryn commented Jul 5, 2017

you sure you're using the latest version? i fixed all seazone/island issues a while ago (like 1.5 years or something when i redid the map). maybe it was lost in the move to github? (or there is a regression in the map or the engine)

@Cernelius
Copy link
Contributor

@veqryn (it was more than 4 years ago) How did you fix them? What I and, I believe, everyone reading the available documentation knows is that, beside islands not being actual islands (that requires showing a line between the island and the sea zone border), the way to assure that is by the territory around the island ending in "Sea Zone". So, what was this other way you followed? Were there a (not documented?) rule of priority based on centers coordinates or anything like that?

@veqryn
Copy link
Member

veqryn commented Jul 5, 2017

the polygons for the seazone exclude the island's polygon (ie: the line method)

@Cernelius
Copy link
Contributor

@veqryn I just tested the old releases since 1.6.1.4 and, from what I see, I believe you must be remembering wrong, or maybe you didn't get around updating to that point. While I see that 1.6.1.4 has still the old version 3.2, I see than in all engines from 1.7.0.3 till 1.8.0.9, all having the same new map version 4.3 installed, SZ 51 and SZ 65 are the same as now (the polygons for the sea zones include the islands' polygons).
So, I don't believe the so called "line method" was ever applied for a released version of this map, but these two couple of islands and sea zones just remained in 4.3 as they were in 3.2.
I also see that both Corsica and Cyprus happened to be selectable in the older engine, having the old 3.2 map version (they were already unsupposedly made back then; just happened to factually work, as Cyprus still does).
What made this existent bug surfacing I believe must have been a change in the 1.7 (from the 1.6) engine, since if I load the 3.2 (pasted from 1.6.1.4) with the 1.7.0.3, Corsica is not selectable, while it was selectable with previous engines, using the same map.
So, we go on till 1.8.0.9 and, from what I see, nothing changed here since 1.7.0.3.
It looks like this bug existed uninterruptedly since 1.7.0.3 (and, actually, existed since ever, but previously it did not happen to affect the map, just like Cyprus still doesn't).
I'm not surprised, considering that this is a seldom played map (but it has a solid following) and very few people ever report any bugs and this problem here is, I guess (I've never played this map), virtually irrelevant, as you can select the starting infantry in Corsica, while Corsica is a value 0 territory of close to no strategic significance by itself, from what I can guess.

However, this conversation has gone far too long for something that can be easily corrected in a few minutes by rewriting the polygons for the sea zone to exclude the island's polygons, both for SZ 51 and SZ 65.

Anyhow, the so called "line method" is really unpolished, thus I suggest a developer to add a map.properties option for defining all territories supposed to be possibly underneath others, defaulting to all territories ending in "Sea Zone" if not set, that should be something like:

map.(...)_territory_names=SZ 51 Ligurian Sea,SZ 65 East Mediterranean

Then update the map.properties of this map accordingly.

If there are no developers that want to do something like this anytime soon, I guess I can pull what needed to address the problem the way @veqryn would do it, tho I don't like that solution.
The leaders here let me know what you prefer, I can pull the line method solution (I dislike) anytime, or anyone else easily can, so this can be closed.
Look at New World Order "sz38" for an example of that method.

@DanVanAtta
Copy link
Member

@Cernelius I'm not sure if I quite follow. Could you confirm if this is still an active problem, or if we still need to apply a fix to avoid the bug on this map?

@Rogue77-NL
Copy link
Author

Hello!

I have just updated from 1.9.0.0.3635 to 1.9.0.0.8304, and I'm now able to move land units to Corsica again, although the bottom left corner of the game window still shows 'SZ 51 Ligurian Sea' when you point the cursor at the island.

However, land territory 'Songea' and sea zone 'SZ 71 Mozambique Channel' are still connected. I'm not quite sure what I'm looking at here..? triplea-maps/great_war@4d534f9. Will this issue be fixed eventually, or should it have been fixed already and am I missing something?

Thanks again for your time!

@Cernelius
Copy link
Contributor

As I said:

  1. This can be easily corrected, with the line method (look at NWO Baleares) (I guess I can do it, tho I have the idea that changes to a map should be done by someone that plays or at least like the map, that I'm not).

  2. The line method is a bad looking workaround (look at NWO Baleares without map details). The problem here and in several other maps is that the only way to deprioritise a territory with respect to another is having the first one ending in "Sea Zone", and I believe relying in any ways on the xml name of a territory to determine its skin-related priority is inherently bad and wrong a process. The current method has also the problem of obscurity, with the consequence that, with it, you have working bugged maps that are improperly made and just happen to work correctly, by chance that the territory that is drawn above the other one is the right one (like here it is the case of Cyprus).

@Cernelius
Copy link
Contributor

I add the info that the reported connection bug has already been solved:
triplea-maps/great_war@4d534f9

@DanVanAtta
Copy link
Member

Sounds like this is solved, is there anything further to be done?

@panther2
Copy link
Contributor

panther2 commented Sep 4, 2018

@DanVanAtta
Yes, the first issue mentioned ("Land territory 'Corsica' is counted as sea zone 'SZ 51 Ligurian Sea'.") has not been completely solved, yet. You can move land units to Corsica, but the island is still identified as "Ligurian Sea".

@Rogue77-NL
Copy link
Author

Hello, why has this issue been closed if according to @panther2 it is not completely solved yet? Kind regards.

@panther2
Copy link
Contributor

@Rogue77-NL See this discussion to learn about the "ice box - close and revisit later" label:
#3960

@Rogue77-NL
Copy link
Author

...a bit late, but thanks again to all for your time and effort. I do like the map, and I appreciate it. :)

@panther2
Copy link
Contributor

@Rogue77-NL The issue has been fixed now. (triplea-maps/great_war#3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants