TriggerAttachments on removed options cause an exception

[trevan]

How can the problem be recreated?

Any map that has a TriggerAttachment that changes an option that was removed in 2.0 will throw an exception and the game stops.

Total World War 3.0 is an example. Its trigger triggerAttachmentgermanRock2 changes the isSuicide option to false. This trigger always runs before the German non combat move. All you need to do is buy a rocket as Germany and the next German non combat move will kill the game.

The issue is that isSuicide is deprecated and is a write only value. So it can't get the original value. And it can't really update it either.

I think 2.x has other options that have been removed, renamed, etc. These will all cause this same issue.

Do you have any ideas for an expected fix?

These options need to proxy their calls to the new settings or just ignore the call.

Attach a Save Game

isSuicideError.zip

There are a few battles left in the combat phase, but once they are done, the error will occur.

If playing a prerelease, which version are you using?

Game Version: Latest on master

If playing a prerelease, does this happen on the latest release?

• yes
• no

Is there anything else we should know?

[trevan]

I know of isSuicide and isSub. Are there other properties? @DanVanAtta

[Cernelius]

Should you change the title, as those are "options", not "properties"?

This is normal, or at least it has always been that way. TWW also had the same problem back then when multiple hipoints were added.

It would be good to make a list of all such cases and check all games in the repository.

[trevan]

I've done a grep through all of the games in the triplea-maps. I looked for where unitProperty and isSuicide/isSub were on the same line. I couldn't find a single map with isSub. I found three maps with isSuicide: HexGlobe FFA w TWW and both TWW versions.

Should the map just be updated? Or should the code be changed to support the old properties through some sort of proxy?

[Cernelius]

Or should the code be changed to support the old properties through some sort of proxy?

In this case, this should be done on every option that sets one or more other options. I believe not all of them are deprecated. At least some of them are to make mapmaking easier. Or are they?

[trevan]

I created #6970 to fix the TWW isSuicide issue. It also wraps the exception to include more details in case there are other options that are causing this issue.

In this case, this should be done on every option that sets one or more other options. I believe not all of them are deprecated. At least some of them are to make mapmaking easier. Or are they?

This only needs to be done for options that are removed (either completely, or by removing their getter or setter). The getter for isSuicide and isSub were removed and they became write only (since they actually set other options). As long as on option has a getter and a setter, then this issue won't affect them.

[Cernelius]

This only needs to be done for options that are removed (either completely, or by removing their getter or setter).

On short term, surely. On long term, I think this is a confusing direction to go, as you would, then, end up with some of such options that can be used this way and some other not. This would require additional documentation (telling that you can do it for isSub, but you cannot do it for a number of other options doing the same thing (setting one or more other options)), easily causing confusion amongst mapmakers, especially new ones. I reiterate the suggestion that either all of them or none of them should be trigger settable, to keep consistency. If this suggestion of mine is not taken, I can only tell to make the matter very clear in pos2 (the game-making guide).

[trevan]

I guess I'm a little confused on what you are saying. I don't see how this makes it confusing. When a map maker builds out the map, there will be certain options that are available. Those options are what should be used. Later, in a newer version, an option might be split (such as the isSub and the isSuicide). The game engine should try and ensure that the old maps still work with the new options but map makers should use the new options and cease to use the old options. The fact that an old option might have a backwards compatibility layer doesn't mean that map makers should use them.

[Cernelius]

I guess I'm a little confused on what you are saying. I don't see how this makes it confusing. When a map maker builds out the map, there will be certain options that are available. Those options are what should be used. Later, in a newer version, an option might be split (such as the isSub and the isSuicide). The game engine should try and ensure that the old maps still work with the new options but map makers should use the new options and cease to use the old options. The fact that an old option might have a backwards compatibility layer doesn't mean that map makers should use them.

Mapmakers may see that a map is using one or more such options this way, thus doing the same with the same or other options, that may or may not work, unless the matter is well documented in pos2. When you are talking of "new options", are you sure that all options that set one or more options are currently deprecated (thus should be just never used) if this is what you are implying. If this is not what you are implying, why are you assuming that mapmakers will not use a non-deprecated option just because it is old or it is setting other options, especially in the moment they may see such usage in other games (TWW) (unless you document it, that it is what I was saying, already).

On an additional note, if we are already not very clear right now that we are discussing the matter, how about mapmakers that may meet this inconsistency (some of such options can be triggered, some other not) at several years of distance from now, knowing nothing of this.

[tvleavitt]

We should have a list of all maps using every still working, but deprecated, option and update them ourselves if the task is simple and doesn't impact play. If there's a design issue, we should ask the creators to update them, and put some kind of time frame on it. Once an option has been removed from all maps, we should remove it from the code. We should list the deprecated options in pos2, and make it clear that newly created maps will be rejected if they have them, and then have a script that scans newly submitted maps for these and notifies the creator to remove them if they're included. This balances backwards compatibility with moving forward and minimizing compatibility code. Thomas <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 Tue, Jun 30, 2020 at 4:02 PM trevan ***@***.***> wrote: I guess I'm a little confused on what you are saying. I don't see how this makes it confusing. When a map maker builds out the map, there will be certain options that are available. Those options are what should be used. Later, in a newer version, an option might be split (such as the isSub and the isSuicide). The game engine should try and ensure that the old maps still work with the new options but map makers should use the new options and cease to use the old options. The fact that an old option might have a backwards compatibility layer doesn't mean that map makers should use them. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6961 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB5HAM6CEABMLAZ72JYJS3RZJVIXANCNFSM4OMNFN6A> .

-- Thomas Leavitt Internet enabled since 1990

[Cernelius]

@tvleavitt I agree, but TripleA has quite a story of losing deprecation information along the way, not documenting them or mapmakers just keeping using deprecated items regardless (a lot!).

For example, how about the "trigger" option? That is the deprecated alternative to "conditions" (I'm almost sure it is deprecated, actually). I don't want to go off topic discussing specific matters, just making an example amongst several.

[trevan]

Mapmakers may see that a map is using one or more such options this way, thus doing the same with the same or other options, that may or may not work, unless the matter is well documented in pos2. When you are talking of "new options", are you sure that all options that set one or more options are currently deprecated (thus should be just never used) if this is what you are implying. If this is not what you are implying, why are you assuming that mapmakers will not use a non-deprecated option just because it is old or it is setting other options, especially in the moment they may see such usage in other games (TWW) (unless you document it, that it is what I was saying, already).

On an additional note, if we are already not very clear right now that we are discussing the matter, how about mapmakers that may meet this inconsistency (some of such options can be triggered, some other not) at several years of distance from now, knowing nothing of this.

Yes, there does need to be a way to notify new map makers that they are using options that have been removed. Is there not a mechanism already?

And I'm only talking about deprecated options that have had its getter or its setter removed from the code base. Those are the ones that will trigger this error. Any other option is not affected.

[tvleavitt]

The process should be automated. The list of deprecated but still supported options should be automatically generated, preferably straight from the code in some way (how, I'd leave up to the developers... I'd presume either via a comment with an agreed up tag of some sort, or use of a common code convention that a script could flag as including a deprecated option)). This keeps the documentation current without effort. Concurrently, automating the process of managing obsolete map content will minimize effort. The number of maps is such that manual updates are tenable, but it is possible that a script could be created to automate the easier fixes; I'm pretty good with Perl and regex. Similarly, a script could be created that builds a list of the maps using deprecated options, and which deprecated options are still in use, and then that list could be automatically compared against the one generated from code, and any options no longer present could have an issue automatically created for the removal of associated code. Regards, Thomas Leavitt <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 Tue, Jun 30, 2020 at 4:38 PM Cernelius ***@***.***> wrote: @tvleavitt <https://github.com/tvleavitt> I agree, but TripleA has quite a story of losing deprecation information along the way, not documenting them or mapmakers just keeping using deprecated items regardless (a lot!). For example, how about the "trigger" option? That is the deprecated alternative to "conditions" (I'm *almost* sure it is deprecated, actually). I don't want to go off topic discussing specific matters, just making an example amongst several. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6961 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB5HAPCGIUXVTHBCYQJ2JDRZJZOHANCNFSM4OMNFN6A> .

-- Thomas Leavitt Internet enabled since 1990

[Cernelius]

Mapmakers may see that a map is using one or more such options this way, thus doing the same with the same or other options, that may or may not work, unless the matter is well documented in pos2. When you are talking of "new options", are you sure that all options that set one or more options are currently deprecated (thus should be just never used) if this is what you are implying. If this is not what you are implying, why are you assuming that mapmakers will not use a non-deprecated option just because it is old or it is setting other options, especially in the moment they may see such usage in other games (TWW) (unless you document it, that it is what I was saying, already).
On an additional note, if we are already not very clear right now that we are discussing the matter, how about mapmakers that may meet this inconsistency (some of such options can be triggered, some other not) at several years of distance from now, knowing nothing of this.

Yes, there does need to be a way to notify new map makers that they are using options that have been removed. Is there not a mechanism already?

And I'm only talking about deprecated options that have had its getter or its setter removed from the code base. Those are the ones that will trigger this error. Any other option is not affected.

@trevan I tried to remain on general terms but, if it helps clarifying further, let's go on with what is specifically mentioned (by you) in this issue.

Here we are talking of two options, namely isSuicide and isSub.

As you can see here:
https://github.com/triplea-maps/the_pact_of_steel/blob/master/map/games/pact_of_steel_2.xml

isSuicide is (has been recently) deprecated (but at least some mapmakers are going to use it anyway).
isSub is not deprecated (as far as I know!). In fact, it is a usable option for setting a number of other options (which is good, as it helps mapmakers and reduces the risk of "map bugs", meaning mapmakers setting something wrong with respect to how they intend the game actually to work).

If you support this way one or more, but not all, of such options setting other options, you add an inconsistency in that some of them can be triggered and some other of them not.

If this inconsistency is limited to deprecated options only (in this case, meaning if you do it for isSuicide but you don't do it for isSub), I guess the matter is relatively less important, but it is still an inconsistency that I hope you will document and, even documenting it, it is still going to be confusing, especially to new mapmakers, in my opinion.

I hope now my point of view is as clear as it can be.

[Cernelius]

@trevan I'm also not sure if you are aware that options setting other options is not only a consequence of deprecation. There are a few non-deprecated (as far as I know) options setting other options that are "shortcuts" for easier mapmaking. isSub is an example, and not the only one.

[Cernelius]

And if I'm wrong about what is deprecated and what not, that probably means the matter is not very clear, or at least there may be mapmakers around that are too. I realize this would be a discussion for another issue, about how to handle deprecation and its information. I can only say that I believe that, under the current conditions, as far as I understand them, deprecated elements may be still used in new games. So, if someone would say something on the line of: "It is deprecated, so it will not be used in any new games anyway: We don't need to worry about this", I would not agree with it.

[trevan]

I was under the impression that isSub was deprecated but you are right that pos2 doesn't list it that way. I modified TWW to also change isSub in the same attachment that isSuicide is being changed and I got the same error. So isSub is definitely broken as well.

What other options were changed/removed/split in 2.0? And what other options are "shortcuts"? @Cernelius

[Cernelius]

What other options were changed/removed/split in 2.0? And what other options are "shortcuts"? @Cernelius

That is not a question I can (or have the autority to) answer. I assume whatever you can find in pos2 that is not defined as deprecated is not deprecated, comprising any options setting one or more other options. On the other hand, I assume that all options that are not documented in pos2 at all (there are at least some) are deprecated too.

As far as I know, but I'm not sure and certainly haven't tested all of them, currently all options that set other options are not trigger settable. I think it would be advisable either all of them or none of them are. I believe it would be confusing (and it would need documentation) if some of them are and some other not.

So, I reiterate my suggestion: Either do this change for all of them or none (just my personal opinion).

Meaning I advise either all options that set other options are settable by triggers or none of them.

[DanVanAtta]

How to handle map properties is tricky. For this case, I would say it's one of the last times we deprecate map options and would have opted to just fix the maps.

We had an example very recently with someone having an error with a map with an attatchment attribute. That misspelling was corrected years ago, yet here we are with an example that is no less than two weeks old.

My thoughts on this are two fold:

1. For better or worse, we probably have to support every map option that was ever in existence and should never let the engine blow up if it see something that is unrecognized.

2. There is no mechanism for indicating a map is an old syntax. We would like to be able to clean up the code, but there is no good mechanism for this. For example, if an attribute is just no longer used, or if an attribute is dropped. Newer versions of the game engine will blow up on this. We do have a min-engine-version so the engine can know if a map is too new, but nothing to tell the engine about old maps that are not properly supported. To this extent adding a versioning to XML I think is the answer. In this manner if the engine see that the map XML version is version "A", it will know to read 'isSubs", but if the version is "B" then it'll know that property should no longer exist. IN this manner we can keep backward compatibility and still allow the code to evolve. More details about what this look like are in an older issue: Map Parsing Version #3104

[Cernelius]

Then, please, clearly document in pos2 this matter: What options that set other options can be set by triggers and what not, and whether or not each of them is usable in new or updated games as a non-deprecated element (obviously, setting isSuicide with triggers is deprecated, since the option itself is, but how about isSub? (can I turn a unit into a sub during the game with triggers or is this a deprecated thing that exists only to support some games already having it?)).

[DanVanAtta]

@Cernelius deprecated options we would rather not document. We don't want to encourage, "Hey, this option which is deprecated, actually triggers these other options, so instead of setting those other options you can use this deprecated one to trigger those as a backdoor".

Keeping the game working is important though.

If any latest options are missing from Pos2, please call them out.

[DanVanAtta]

With that said, having a list of deprecated options somewhere would be pretty reasonable. I think we might be stretching the utility of Pos2 if we do that in the same XML.

Pos2 I think is meant to be a source for copy-paste and then edit, not necessarily to demonstrate which options are old. It's therefore more important that it be kept to the latest.

[Cernelius]

@Cernelius deprecated options we would rather not document.

This is how it was done in the past (and this is why we have a few options that are usable but are not mentioned in pos2 nor anywhere else, as far as I know, thus nobody can really know that they are deprecated). Then, instead, @ron-murhammer started documenting deprecated options in pos2. So now we have some deprecated options that are completely missing in pos2 and some deprecated options that are documented as deprecated in pos2. And now, on top of this, this matter has been added, that I hope will be clearly documented somewhere, especially for anyone that didn't follow this issue.

I already said that isSub is not deprecated, as far as I know. If I'm wrong, please let me know. Since (or should I say "if") the option itself is not deprecated, how can anybody know if setting it by triggers is deprecated or not?

[DanVanAtta]

We could document isSuicide as deprecated. Learning about deprecations through reading Pos2 does not seem wise though. Pos2 is meant to be an example map, not the source that seasoned map makers go to look for changes. It seems something else should serve that purpose (perhaps simple forum posts or a wiki page)

[Cernelius]

We could document isSuicide as deprecated.

If you mean in pos2, it already is (as I already said).

[Cernelius]

We could document isSuicide as deprecated.

If you mean in pos2, it already is (as I already said).

Though, it would be good to explain that this option is exceptionally settable by trigger, differently from all other options setting other options, to support existent games, albeit this possibility should not be used in new or updated games.

Am I understanding correctly that isSub has been merely mentioned as an example by @trevan, but there are no actual known problems currently related to it?

[trevan]

Am I understanding correctly that isSub has been merely mentioned as an example by @trevan, but there are no actual known problems currently related to it?

isSub is broken when used as a trigger. I was able to cause this error with isSub in a modified map. It will need to be fixed. The change that got merged (and which triggered the closure of this bug) improves the error message so we know which option is not working as a trigger.