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

TripleA 2.6.895 gives this error - Could not find step name #10647

Closed
TheDog-GH opened this issue Jun 12, 2022 · 9 comments · Fixed by #11773
Closed

TripleA 2.6.895 gives this error - Could not find step name #10647

TheDog-GH opened this issue Jun 12, 2022 · 9 comments · Fixed by #11773
Labels
2.6 Problem A problem, bug, defect - something to fix Regression A problem introduced in pre-release and not present in latest release

Comments

@TheDog-GH
Copy link
Contributor

This map works without error in 2.5

But 2.6.895 has the error below, by taking the defaults, it does not get past round 1.

With 2.6 it does not give me a proper error message or clue me where to look, please take a look. The step error should work with uppercase Finns and Russians, yes?

image

Map
WW2_Russia_Finland_1939.zip

@asvitkine asvitkine self-assigned this Jun 13, 2022
@asvitkine
Copy link
Contributor

So it's referring to steps in a battle. The list of steps the engine has in that situation is:

  • Finns select casualties
  • Finns notify casualties
  • Finns units Elite fire
  • Russians select units Elite casualties
  • Russians notify units Elite casualties
  • Finns units Forest fire
  • Russians select units Forest casualties
  • Russians notify units Forest casualties
  • Remove casualties
  • Russians withdraw?

So "Russians select casualties" is not one of them. I guess there's some customization of the step names somewhere and some code might not expecting for that to be the case and is looking for a normal casualties step?

@asvitkine
Copy link
Contributor

asvitkine commented Jun 13, 2022

The step name comes from getPossibleOldNameForNotifyingBattleDisplay() which was added in this 2.6 PR:
b804d37

It does a remapping of a step name, for example:
"Finns select air vs non subs casualties" gets mapped to "Finns select casualties"

Because the original string wasn't in the list of:

[Finns air vs non subs fire, Russians select air vs non subs casualties, Russians notify air vs non subs casualties, Finns fire, Russians select casualties, Russians notify casualties, Russians units Artillery fire, Finns select units Artillery casualties, Finns notify units Artillery casualties, Russians units Mountain fire, Finns select units Mountain casualties, Finns notify units Mountain casualties, Remove casualties, Finns withdraw?]

But the step name it returns is also not in the list!

I'm really not familiar with this code, so maybe @trevan could chime in or @DanVanAtta who reviewed the above PR?

@asvitkine asvitkine added Problem A problem, bug, defect - something to fix Regression A problem introduced in pre-release and not present in latest release labels Jun 13, 2022
@TheDog-GH
Copy link
Contributor Author

TheDog-GH commented Jun 13, 2022

This is the first time/map I have used canNotTarget and if I comment out its use the map runs without error.

As an aside

Finns units Forest fire
Russians select units Forest casualties

Forest (EDIT & Mountain) are isInfrastructure units with no attack or defence, so should they even be in this list?

It might also be worth looking at canNotBeTargetedBy in the above context ?

@asvitkine
Copy link
Contributor

Indeed, canNotBeTargetedBy is relevant here.

The distinct step names above (e.g. "Russians units Mountain fire") are generated by newTargetGroups(), which splits on getCanNotBeTargetedBy() via findTargets().

I agree that the result doesn't make sense given the units don't ever fire or participate in combat! But I'm not sure how exactly the logic changed here in 2.6 and what's the right fix (I mean, we can bandaid it by filtering units, but seems like there might be a more fundamental assumptions mismatch between different parts of the battle code). I'm hoping @trevan could chime in.

@trevan
Copy link
Contributor

trevan commented Jun 21, 2022

That error pops up in a bunch of different locations. It existed before my changes and, as this bug shows, still shows up.

It actually doesn't break anything. What is happening is that when a step occurs, the UI tries to match the step from the engine with the step in the UI. They are supposed to match but are created at two different times. The UI steps are created at the very beginning of the battle while the engine creates each step as it is needed. It is possible for the UI to have steps that the engine doesn't have and vice versa. When the engine has a step that the UI doesn't have, that error will occur. But, you can press "OK" and keep going and nothing will break.

The getPossibleOldNameForNotifyingBattleDisplay method was an attempt to migrate old step names to the new step names that I added. It will only fire when you open a save game from an older engine. If the save is from 2.5 and being opened in 2.6, then that method will be called, but if the save is from 2.6, it should never be called. It sounds like the save is a 2.6 only save, so that method is not where you would want to look.

@DanVanAtta DanVanAtta added this to Triage in Problem Tracker via automation Aug 20, 2022
@DanVanAtta DanVanAtta moved this from Triage to Back Log: Fix Sooner in Problem Tracker Aug 20, 2022
@asvitkine asvitkine added the 2.6 label Jul 4, 2023
@asvitkine
Copy link
Contributor

Dupe of #11617

@asvitkine
Copy link
Contributor

asvitkine commented Jul 12, 2023

So, looking at this more, one thing that changed is the actual list of steps between 2.5 and 2.6.

2.5:
2 5
2.6:
2 6

So 2.6 is splitting and including a step for forests, even though they don't participate in combat (don't show in the unit list).

So I think that would be the first thing to fix.

@asvitkine asvitkine reopened this Jul 12, 2023
@asvitkine
Copy link
Contributor

(Repro is very simple on the above map, just play as a human and attack Finns in Ladoga.)

asvitkine added a commit to asvitkine/triplea-1 that referenced this issue Jul 12, 2023
This was happening because the code to generate step names was not excluding units that would not participate in combat, resulting in infrastructure units getting their own steps (which later did not match what the engine generated once the filtering took place).

Uses the same logic as what's done for the battle to exclude units.

Fixes: triplea-game#10647
@asvitkine
Copy link
Contributor

The following PR appears to fix this: #11773

I need to do more testing and add a unit test.

asvitkine added a commit that referenced this issue Jul 15, 2023
This was happening because the code to generate step names was not excluding units that would not participate in combat, resulting in infrastructure units getting their own steps (which later did not match what the engine generated once the filtering took place).

Uses the same logic as what's done for the battle to exclude units.

This change required adjusting a bunch of tests that were previously not careful about which had mistakes in setting up mock battles where the units didn't match the territory (in terms of sea vs. land). Also makes the tests to mock game data properties leniently, so that only the ones being set to true need to be specified (removing lots of LOC).

Includes a unit test.

Fixes: #10647
Note: Doesn't fix  #11617, as the root cause of that error is different, despite the actual error being the same. I will fix that in a separate PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.6 Problem A problem, bug, defect - something to fix Regression A problem introduced in pre-release and not present in latest release
Projects
Status: Done
Problem Tracker
Back Log: Fix Sooner
Development

Successfully merging a pull request may close this issue.

3 participants