Add Support for (most) 1.8 maps #7631
Conversation
This initial round of updates allows for basic support of 1.8 maps, they will load after this update.
- Add conversion code to match attachments spelled either as 'attatchment' or as 'attachment - Add annotation @RenameOnNextMajorRelease to help us know which items we can rename and to what when save game compatibility can be broken. - Add ignore for properties: "takeUnitControl" and "giveUnitControl" Add conversion logic for: - isParatroop -> isAirTransportable - isInfantry & isMechanized -> isLandTransportable - Battleship|Units repair at beginning|end of round -> Units Repair at beginning|end of round - 'occupiedTerrOf' -> 'originalOwner' - 'victoryCity=false' -> 'victoryCity=0' - 'isImpassible' -> 'isImpassable' - 'turns' -> 'rounds'
Codecov Report
@@ Coverage Diff @@
## master #7631 +/- ##
============================================
+ Coverage 24.32% 24.38% +0.06%
- Complexity 7084 7129 +45
============================================
Files 1211 1212 +1
Lines 79025 79069 +44
Branches 11319 11330 +11
============================================
+ Hits 19221 19282 +61
+ Misses 57634 57612 -22
- Partials 2170 2175 +5 Continue to review full report at Codecov.
|
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
game-core/src/main/java/games/strategy/engine/data/gameparser/LegacyPropertyMapper.java
Show resolved
Hide resolved
game-core/src/main/java/games/strategy/engine/data/gameparser/LegacyPropertyMapper.java
Show resolved
Hide resolved
| return mapXmlToObject(pojo, pojo.getSimpleName()); | ||
| } | ||
|
|
||
| public <T> T mapXmlToObject(final Class<T> pojo, final String tagName) |
There was a problem hiding this comment.
Codacy found an issue: Avoid really long methods.
| private void setPlayerAttachmentName(final String playerAttachmentName) | ||
| throws GameParseException { | ||
| if (playerAttachmentName == null) { | ||
| this.playerAttachmentName = null; |
There was a problem hiding this comment.
Codacy found an issue: Assigning an Object to null is a code smell. Consider refactoring.
|
I strongly disagree with this PR (beside the fact that I believe several items, as proposed, don't actually realize the 1.8 behaviour). In case, at least be sure that, while the map is working, errors will be given, so that the use of deprecated items won't be possibly overlooked (I believe this should happen for every deprecated item, not only those that this PR would add). There is also still this matter, that was never addressed, as far as I know (with special reference to the |
|
@Cernelius , I'd encourage you to share your opinions in a more constructive way and less opinion but more the problems you would foresee. Are you concerned that people will not upgrade their maps? My response is that even after 4 years we still see regularly 1.8 maps being used and crashing on players. Is there to be no solution other than "sorry, that map just does not work?". While it would be great for all maps to be using the latest configuration, it's not a reality. When we upgrade XML further to be clearner and more expressive, it'll be a reality that most maps won't be upgraded. Is there a reason to really require that? Aesthetically nice XML that is the latest is great, but it does not change that most maps are only maintained by the admins and are not owned by anyone. |
Can you clarity this @Cernelius ? Can you point to any specific 1.8 maps where this can be tested?
That can perhaps be done; but how does that help when someone pulls a map that is 5 years old and they are just a random player and are 100% uninterested in updating the map? Whom would such errors serve? Those maps are never going to be updated. If a map uses a deprecated item, why is that a problem? I think we should try to make it so new maps use the latest, but if a map is once working, what's the value in making it not work anymore because we say "no, you calling it 'turns' instead of 'rounds' displeases us, here is an error and the game is now crashed." I'll note that before recently and a substantial amount of work there was no hope for the map to work at all. Using deprecated items hard-crashed the game, now we can deal with it more easily so at least the game does not hard crash if it runs across a deprecated item. |
Since my advice is not to do this, it would be useless to suggest corrections to it. As the "beside" implies, I'm not against it because of the matter. |
|
Here's my suggestion:
We obviously know what's wrong with the maps and have the code to
automatically fix them, if we can do that dynamically when they're loaded.
Pull the known ones down from wherever. Run them through a program to fix
them that does exactly what this PR does. Stick them in the repo. Next time
someone fires up TripleA, they'll see a notification to update the fixed
map. We get more maps that work, which is good. Users no longer get error
messages trying to load many of these older maps.
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon>
Virus-free.
www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link>
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
…On Sat, Sep 12, 2020 at 1:38 PM Dan Van Atta ***@***.***> wrote:
(beside the fact that I believe several items, as proposed, don't actually
realize the 1.8 behaviour).
Can you clarity this @Cernelius <https://github.com/Cernelius> ? Can you
point to any specific 1.8 maps where this can be tested?
In case, at least be sure that, while the map is working, errors will be
given, so that the use of deprecated items won't be possibly overlooked
That can perhaps be done; but how does that help when someone pulls a map
that is 5 years old and they are just a random player and are 100%
uninterested in updating the map? Whom would such errors serve? Those maps
are *never* going to be updated.
If a map uses a deprecated item, why is that a problem? I think we should
try to make it so new maps use the latest, but if a map is once working,
what's the value in making it not work anymore because we say "no, you
calling it 'turns' instead of 'rounds' displeases us, here is an error and
the game is now crashed." I'll note that before recently and a substantial
amount of work there was no hope for the map to work at all. Using
deprecated items hard-crashed the game, now we can deal with it more easily
so at least the game does not hard crash if it runs across a deprecated
item.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#7631 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABB5HAJIPZ2CGGTNLDMTWKLSFPL4FANCNFSM4RJGWAJQ>
.
--
Thomas Leavitt
Internet enabled since 1990
|
|
I assumed it was obvious, but, yes, I believe supporting deprecated items just means that these items will keep being used. In the moment all repository maps are supposed not to have these items and new maps added to it are certainly supposed not to have them I see no good enough reason for supporting deprecates. I think all deprecates should be removed as soon as no maps in the repository currently have them. If someone uses a no longer supported item (as a user downloading a map not through TripleA), a warning should clarify it that the item is not currently supported by TripleA and it may be an item that was supported in the past or never. If the map is currently available in TripleA, the user should be prompted to update it. For example, look at this: That is a new coding and the deprecated "artillerySupportable", "isInfantry" and "artillery" are being used (and that's just 3 deprecated items over 2 attachments). |
This is actually a very recent feature. Previously DTD validation would have killed these maps, beyond the code not existing to support legacy options. We dropped the legacy support because the conversion code was too overwhelming. Now that we do map loading in phases, it's much easier to either "fake-out" legacy properties as new ones. The invasive changes here are to string replace the bad 'attatchment' spelling to 'attachment', though that stays pretty minimal.
There are two problems here:
I'd raise a larger issue, what's the point? Why should we force this work on anyone? It's complete, 100% busy work for no benefit. If the engine can shim in code to let a legacy map work, why should anyone take time to update? I mean if the map is in-progress, and new, then sure, use the latest. But if we are talking about a map that has been sitting in a repository for a decade, has not changed, will not change, then why force the labor to change it? There is some backstory, it was never intended that we say to old maps "screw you, you don't work anymore, download an old game engine." The game engine used to contain all previous major versions as bundled jar file and the game engine would use java.exec to launch those jars as needed to run the old maps. This is incredibly dirty and does not scale, we removed that and until know never built in a better way to support running legacy maps. In essence, TripleA will need to always support any major map version that has been created indefinitely. Bundling 'old game' versions was an approach, but not scalable and hacky (and still ran into DTD problems as well). The new architecture for map loading gives us a lot of new freedom and flexibility for cleanly support differing formats of XML and removes the long-standing DTD problem: #7633
This is my point, we're assuming that person would care or be in a position to do that. My preposition is that 100% of map makers are focused on only a few maps and most of these situations is some random player finding an archive of old maps and giving them a try. Such a message to them would be pure noise and never get done. Above, I'd even question, why should it even be done (assuming there was someone aware and even willing to do so)? Perhaps of note, writing a latest format right now is not possible. All upgrade modifications need to be done by hand. Having something write out a latest XML is an extremely desired feature and would naturally lead to an upgrade type-of program which would be an excellent thing to have beyond the capability that gives for easier map making (which is an incredibly compelling reason to want that). There is currently a 'map-xml'writer' built in, but it's cludgy, produces invalid values, and until recently you could not even load an old map to then get to that point (until recently these old maps hard-crashed the application, hence the various error reports about "attatchment" being an invalid property. Until literally this PR, those maps hard-crashed the application). |
I agree this is a potential issue. On the one hand, if the map works, is it really a problem? Arguably not, just not clean. On the other hand, I think we need to focus on how to most concisely and simply document "here is how you build a map" and focus on giving the latest best practices. As the XML continues to evolve, we can't expect to update all XMLs. For starters we do not know or control all XMLs that are inexistence. For second, the proof of history is against us. Even straight forward changes to the XML has been virtually a disaster. The upgrade from 1.8 to 1.9 was incredibly painful due to map issues. Please see my comment above, it's always been a design choice that triplea should always load old maps - always. We can't reliabily count on them being updated, and it's too much work frankly to try and do that for no benefit other than "ah, you're using the cleanest and latest XML, this is nice" (aesthetic).
Indeed, yet we still see regular errors about 1.8 maps. Presumably this is coming from non-repository maps, which is where our best intention of maintaining those maps to always be latest breaks down. Again though, the experience of 1.8 to 1.9 maps, updating all maps was not an easy effort, and after we did that basically I said "this did not go well, it was not worth doing, we should not do this again". Which left us in a bad place as well as we had removed the "old jar" functionality so we break a long standing practice of not breaking maps. 1.9 was the first release that simply broke all old maps. The idea that we can always update all maps has been proven false, we can't, we don't own them all, we are not aware of all of them, and the ones in sourceforge even we can't update eve if we wanted to.
This assumes the user will care to do that, or even be able to. I'd suggest most of the time these are random players who give no concern to these messages, they just say "oh well, this game sucks and this map does not work", and nothing further. Until now the game hard-crashed in such a situation,so not requiring maps to be updated or not working, I think we're in a better place quite objectively. I think what we need is probably a validation program that would help find and debug map errors, and it would be that validation program that prints out warning messages. This is something that is used by a map maker, who would care to fix such a problem, and is a tool that is useful beyond getting out of deprecated features but getting maps to work in the first place. |
|
@Cernelius funny enough, this PR does not make this problem worse:
We're actually in a better place to fix that now even. Until the recent work to change the map loading architecture, the game hard crashed. That essentially meant that we were just not going to change the code, period. The existing legacy XML elements, yeah, that is what they would be, for now and forever, no changes, it's frozen. The cost to fix it was too high and for too little benefit, crashing maps is a hard no. So we've now a way to have migration code, so at least we won't crash maps, this means we can start to change the XML structure to be better. This also means if we create a validation program we can give those desired warnings about legacy features. On the other hand, since TripleA is committed to always support all major map formats (it always was, we cheated in 1.9 to violate that and it's long overdue to fix it), it'll always be a problem that legacy features would work. The big benefit recently though is that we can code it so that the legacy feature doesn't overwhem the code and can be cleanly handled. I think the solution to the legacy attribute/option problem is a map validator. |
|
Perhaps something got lost in translation:
I'm proposing that for maps we know we can make work via the engine this
way, to pull them down somewhere, run this same filter against them to
produce a "fixed" version, and add them to the official repository. No
contacting authors, attempting external site updates, etc. Just added
working maps to our repo (a win I think), that in turn triggers a prompt to
download an updated version, just like any other updated map. This PR would
be useful but rarely executed--only when an old map we haven't yet fixed
gets loaded, or someone deliberately ignores the map update prompt. In the
former case, maybe we can have this trigger a "bug report" to us that would
include the map in question, so we can fix and add it to the system.
Yes, a small amount of work to run the filter against the older maps, but
surely no more than you've done to make this PR work by identifying a set
of 1.8 maps, using them for testing, and writing new code. I'd guess less,
since anyone could do it, given a copy of the map and a standalone program
(small amount of new work to create) that updates an old map.
Or perhaps we simply disagree on the value of new maps being updated and
added to our repo, which is fine.
Thomas
…On Sat, Sep 12, 2020 at 2:21 PM Dan Van Atta ***@***.***> wrote:
@tvleavitt <https://github.com/tvleavitt>
We obviously know what's wrong with the maps and have the code to
automatically fix them, if we can do that dynamically when they're loaded.
This is actually a very recent feature. Previously DTD validation would
have killed these maps, beyond the code not existing to support legacy
options. We dropped the legacy support because the conversion code was too
overwhelming. Now that we do map loading in phases, it's much easier to
either "fake-out" legacy properties as new ones. The invasive changes here
are to string replace the bad 'attatchment' spelling to 'attachment',
though that stays pretty minimal.
Pull the known ones down from wherever.
There are two problems here:
1. One location is sourceforge. I don't think anyone has the 'write'
access required to update these.
2. We simply do not know what all locations there are even are.
RogerCooper has a vast repository of maps, beyond that, it's hard to know
what they are.
I'd raise a larger issue, what's the point? Why should we force this work
on anyone? It's complete, 100% busy work for no benefit. If the engine can
shim in code to let a legacy map work, why should anyone take time to
update? I mean if the map is in-progress, and new, then sure, use the
latest. But if we are talking about a map that has been sitting in a
repository for a decade, has not changed, will not change, then why force
the labor to change it?
There is some backstory, it was *never* intended that we say to old maps
"screw you, you don't work anymore, download an old game engine." The game
engine used to contain all previous major versions as bundled jar file and
the game engine would use java.exec to launch those jars as needed to run
the old maps. This is incredibly dirty and does not scale, we removed that
and until know never built in a better way to support running legacy maps.
In essence, TripleA will need to always support any major map version that
has been created indefinitely. Bundling 'old game' versions was an
approach, but not scalable and hacky (and still ran into DTD problems as
well).
The new architecture for map loading gives us a lot of new freedom and
flexibility for cleanly support differing formats of XML and removes the
long-standing DTD problem: #7633
<#7633>
Next time someone fires up TripleA, they'll see a notification to update
the fixed
map.
This is my point, we're assuming that person would care or be in a
position to do that. My preposition is that 100% of map makers are focused
on only a few maps and most of these situations is some random player
finding an archive of old maps and giving them a try. Such a message to
them would be pure noise and never get done. Above, I'd even question, why
*should* it even be done (assuming there was someone aware and even
willing to do so)?
Perhaps of note, writing a latest format right now is not possible. All
upgrade modifications need to be done by hand. Having something write out a
latest XML is an extremely desired feature and would naturally lead to an
upgrade type-of program which would be an excellent thing to have beyond
the capability that gives for easier map making (which is an incredibly
compelling reason to want that). There is currently a 'map-xml'writer'
built in, but it's cludgy, produces invalid values, and until recently you
could not even load an old map to then get to that point (until recently
these old maps hard-crashed the application, hence the various error
reports about "attatchment" being an invalid property. Until literally this
PR, those maps hard-crashed the application).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7631 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABB5HAKV4BOSA3LJRMXUPZTSFPQ6ZANCNFSM4RJGWAJQ>
.
--
Thomas Leavitt
Internet enabled since 1990
|
We don't know where those maps are. The error reports for "attatchment" that we are getting, I've no clue where users are finding those maps. I think that will always be the case,. Second, the conversion logic works going from XML to GameData (java code). The opposite direction does not work cleanly yet. The code to 'write XML' is hacky, incomplete, and actually broken (it produces non-working code, there is a bug report open about this). So this is only a shim to allow legacy features to be read. Please see the notes on the architecture update for how this was enabled in: #7633, and #7498
There are hundreds of maps, the amount of effort for people to deal with source control systems is insane. I can't emphasize how much hatred there is when we told map-makers "hey, github is all self service, this is easy", and there's been a lot of push-back. Before the admins did all of the source control work on behalf of map makers, that was not scalable. More to the point, there are hundreds more maps not in the repositories, adding them is loads of work. |
|
@tvleavitt , I think there are a few disconnects:
|
|
@Cernelius , @tvleavitt , let me see if I can talk this through:
After this update, the game engine no longer hard crashes and just runs. @Cernelius is suggesting that not only should we do this update, but we go further and then hard crash the game engine with a reasonable and exact error message. @tvleavitt is suggesting to instead of artificially hard crashing, to give a warning message instead. The problems I see with either suggestion are:
I think the solution to either of ya'lls concerns/suggestion is a map validation program that would be run by map-makers. That program would give clear error messages about various other problems. One of the bigger problems in map making is nonsense error messages for unknown reasons. There are a lot of help threads about these kinds of situations. That same validation program can also give upgrade prompts to the map maker about how they can use the latest XML and avoid legacy options. In any case, this update is a pre-requisite to any of those paths. The game engine cannot hard crash if it is going to be able to do anything, like give a good upgrasde message or anything else. It's been a longstanding requirement for TripleA to support all maps, that was broken in 1.9, 1.10 and in 2.0. I think the reality has shown that as much as we'd like to think there will be labor and willingness to always update very map, it's not a reality. Some maps will always just be what they are, written 10, 15 years ago and never updated again. Allowing those maps to just work I think is a requirement. We can decide that perhaps we can loosen that, that we can legitamtely say, "this map is too old". But even then, it's difficult to know for the engine if a map is just old, or just broken and is a work in progress. We wouldn't want map makers to see the random error message "your map is too old, throw it away, you can't play it" whenever they make a mistake. Furthermore, I think that is a poor UX for players running across old maps and loading them up. Last, there are actually lots of those maps out there, in unknown locations (and that's another problem we can't readily solve either, there probably will always be maps in unknown locations, out of our control). |
| final boolean clearFirst) { | ||
| this.attachmentName = | ||
| Optional.ofNullable(attachmentName) | ||
| .map(name -> name.replaceAll("ttatch", "ttach")) |
There was a problem hiding this comment.
I mean we all know why this exists, but it would be nice documenting it here in the code nevertheless
| @@ -175,8 +175,7 @@ private static void handleMissingObject(final String objectTypeName, final Strin | |||
| final GameData gameData) { | |||
| checkNotNull(typeName); | |||
|
|
|||
| final String normalizedTypeName = | |||
| typeName.replaceAll("^games\\.strategy\\.triplea\\.attachments\\.", ""); | |||
| final String normalizedTypeName = typeName.replaceAll("^.*\\.", ""); | |||
There was a problem hiding this comment.
Not sure if this change is working as intended.
The reason why the code was like this in the first place is because there is some ambiguity regarding the naming of attachments. "twoIfBySea" has a different namespace unfortunately.
See https://forums.triplea-game.org/topic/595/short-attachment-names for more information
There was a problem hiding this comment.
We'll need to test and fix that if needed. Having an 'or' regex is not really ideal, but we could do it. IMO the java-class designation ideally would be completely removed, but we'll need to support it going forward. Perhaps we could do just straight up hardcoded mappings. We still have reflection going on to create classes by name, an explicit mapping with explicit construction code is I think where I'd like to go (and then couple that with updating the XML so that java classes are inferred on the backend and no longer needed in the XML).
| return Optional.ofNullable( | ||
| getColorProperty(PROPERTY_COLOR_PREFIX + Constants.PLAYER_NAME_IMPASSABLE)) | ||
| .or(() -> Optional.ofNullable(getColorProperty(PROPERTY_COLOR_PREFIX + "Impassible"))) | ||
| .orElseGet(defaultColors::nextColor); |
There was a problem hiding this comment.
Off-Topic: I don't think we have a nice error message for "too few default colors" yet, so that might be something to consider here
commit cca0406
commit 56e0ce4
Testing
Screens Shots
Additional Notes to Reviewer
The list of the 1.8 to 1.9 changes is in: #1033
Release Note
FEATURE|Build in support to be able to load (most) 1.8 maps. 1.8 "variant" maps will not work, though most others should now load without issue.