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

Error with "canNotTarget" #11617

Open
WCSumpton opened this issue May 3, 2023 · 24 comments · Fixed by #11773 or #11780
Open

Error with "canNotTarget" #11617

WCSumpton opened this issue May 3, 2023 · 24 comments · Fixed by #11773 or #11780
Assignees
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

@WCSumpton
Copy link
Contributor

Using engine version: 2.6.14357
Windows 10

In the unitAttachment "isSea" is false and "isAir" is false and using "canNotTarget" produces the following error: "WARN g.s.triplea.ui.BattleStepsPanel - Could not find step name:(attacking player) select casualties"
CannotFindError
In the following save PoS2 game, 'infantry' unitAttachment was modified:
(did not accept save game file)
After loading save game, set Russian to Human and play. Select "done" to end combatMove, and select "Ukraine S.S.R." for your battle. After the error message is cleared, Russian casualties can be selected normally, so the error does not prohibit the selection of casualties, nor does it stop game play.
The following was added:
(did not accept save game file)
Do the same as above, not their is no error now.

Cheers...

@WCSumpton WCSumpton added the Problem A problem, bug, defect - something to fix label May 3, 2023
@WCSumpton
Copy link
Contributor Author

ProblemWithcanNotTarget.zip
First file ProblemWithcanNotTarget.tsvg is used to show the error. Next file: ProblemFirstStrike.tsvg is with isFirstStrike added.

@SupRemeviber
Copy link

Check you code and recode

@TheDog-GH
Copy link
Contributor

This Problem could do with a 2.6 label/tag otherwise it might get missed.

@TheDog-GH
Copy link
Contributor

TheDog-GH commented Jul 5, 2023

@asvitkine
This is still an issue.
Having download TripleA-2.6+14388 and running the latest version of 1941 Command Decision v120
https://drive.google.com/file/d/1Q5XcTxMNdUZVAjjTWbz4mw0gL-Bjlw6r/view?usp=sharing

It still errors in about 1 minute. Set all players to FASTAI.
This does not error in 2.5

ps. this is not the same error as #11492
An unexpected error occured!
This is a warning error

@asvitkine
Copy link
Contributor

I think same error as this one:
#10647

I'll dupe that one over to this one, but note that there was some good discussion on that one too.

@asvitkine asvitkine added the Major Indicates the problem is very impactful, very noticable, no work around label Jul 6, 2023
@asvitkine
Copy link
Contributor

I'm actually going to dupe this in the other direction. closing this as a dupe of #10647 since the other issue already has more investigation done.

@asvitkine asvitkine self-assigned this Jul 14, 2023
@asvitkine
Copy link
Contributor

Actually, keeping this separate from #10647, as the symptom is the same, but the cause is not.

In the other one, it's because we were considering infra units as participants in combat for determining step names.

In this case, at least from the repro provided by WCSumpton, I think what may be happening is that the step names are determined by the first unit from a list of units, but then this can change if there are casualties, such that the first unit will be a different one for that group. But I need to debug more to confirm.

@asvitkine asvitkine added the Regression A problem introduced in pre-release and not present in latest release label Jul 14, 2023
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.
@asvitkine asvitkine reopened this Jul 15, 2023
@asvitkine
Copy link
Contributor

OK, I believe this is fixed, but I would appreciate if folks can test more as there were at least two different causes of this issue. There may be others that I missed.

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

Per the forums, looks like there are yet more causes for this error which still happens on the 1941 CD map:
https://forums.triplea-game.org/topic/2999/2-6-release-getting-close-need-volunteers-to-help-beta-test-2-6/89

Here's one scenario that still causes an error:

  • An attack on a territory defended by a Bomber-Tac, a Fighter-Early and a Fighter.
  • Since Bomber-Tac can hit ground units and Fighters can't the defense dice rolls get split into multiple steps ("Britain air vs non subs Bomber-Tac fire" and "Britain air vs non subs Fighter fire" with corresponding select casualty steps).
  • However, on this map, air units get hit by offensive AA at the start of combat, which gets two hits and Bomber-Tac and Fighter-Early are taken as casualties.
  • Now, when the engine gets to the point of the defensive roll, there's just one unit type ("Fighter") rolling, so code is looking for the simplified "Italy select casualties" step, whereas the steps list has the other steps from before ("Italy select air vs non subs Bomber-Tac casualties" and "Italy select air vs non subs Fighter casualties").

@asvitkine
Copy link
Contributor

Given the above, I wonder if trying to find the step by name is just the wrong approach. We could just track the attacking units for the corresponding casualty steps and match by those.

@TheDog-GH
Copy link
Contributor

I think I understand where you are going with the above.

Let me setup some set piece battles and see if I can work out the cause.

This will take awhile, I will get back to you with a post here.

@asvitkine
Copy link
Contributor

asvitkine commented Jul 17, 2023

Some more info:

  1. The step names in the UI get created early on (MustFightBattle.determineStepStrings()), which actually ends up expanding the full tree of steps, figuring out all the different pairings, etc, only to throw that information away and just keep the step names. (It's actually an expensive operation and shows up in profiles.)
  2. But when it comes to actually executing the battle, we determine the low-level steps as we go, so for example the specific casualties step gets created via:
	at games.strategy.triplea.delegate.battle.steps.fire.general.FiringGroupSplitterGeneral.apply(FiringGroupSplitterGeneral.java:41)
	at games.strategy.triplea.delegate.battle.steps.fire.FireRoundStepsFactory.createSteps(FireRoundStepsFactory.java:37)
	at games.strategy.triplea.delegate.battle.steps.fire.general.DefensiveGeneral.getSteps(DefensiveGeneral.java:68)
	at games.strategy.triplea.delegate.battle.steps.fire.general.DefensiveGeneral.execute(DefensiveGeneral.java:50)
	at games.strategy.triplea.delegate.ExecutionStack.execute(ExecutionStack.java:34)
	at games.strategy.triplea.delegate.battle.MustFightBattle.fight(MustFightBattle.java:724)
	at games.strategy.triplea.delegate.battle.BattleDelegate.fightBattle(BattleDelegate.java:240)

So in this case, the "parent" step (DefensiveGeneral) executes and then generates its child steps, but not based on what the units were initially, but based on what they are at this time. So then, it generates a different step name for casualties than what the UI expects (i.e. if there's only one casualty step needed, it generates the generic name, whereas UI may have had multiple specific ones).

In an ideal world, I think it would be best to generate all the steps (not just their names) at the start and go through them for the battle. If a step becomes no longer relevant, it can just be skipped. But this is a big refactor that I don't want to do now since it would add a lot of risk to the 2.6 release.

Instead, I think I have a solution which is to still save off some relevant information (i.e. firing units for each step) at the time we generate their names. Then, when we encounter the scenario where we can't find the step name, we can use that information to figure out which step it was before (via looking at the new set of firing units and check if they're a subset of one of the previous ones and getting the corresponding step).

However, I think the above solution may still have holes, because I'd imagine we can get to a situation where attacking units did not change, but defenders did in a way that results in a split attack getting combined. In such a case, the UI would show two steps, but we're rolling them together.

I guess yet another option would be to update the UI when the step list changes. However, it's not clear if this is feasible given that may require a network protocol change which we can't do in 2.6. Sigh.

@asvitkine
Copy link
Contributor

@TheDog-GH
Thanks for offering to set something up that can repro this consistently. It will definitely be very helpful to test this properly -as currently the saved game is very random whether it repros it, and because it's run as AI, I'm also missing observing exactly the effect on the UI as this happens.

@TheDog-GH
Copy link
Contributor

TheDog-GH commented Jul 17, 2023

@asvitkine
You have already fixed/reduced the number of warning messages "Could not find step name", by a massive factor.
Looking back through the Triplea.log some of the very high warning messages are;
116, 104, 122, 164, 132, 112 these are the top lot from over 30 games

Using TripleA-2.6+14415 in a test game today 3 whole rounds and no warning messages, but on the 4th my 1st warning messages. This game was 11 rounds with only 8 warning messages. Looking at the time stamps of the warning messages, this might only be 3 actual warning/errors. Also the time stamps warning lines are always in pairs, so the actual number of real warning with be halved.

This was a test for rebalancing the 1941 CD map, with 12 extra USSR units because of your TripleA changes, I will need to add more as Germany easily crushes USSR.

So I would say for me this has gone from a major to a minor issue. (100+ warning to 8)

Meaning you/we should concentrate on other issues to get 2.6 out. :)

However, I will spend some time later this week trying to reproduce a setup with much greater occurrence for these warning messages.

@TheDog-GH
Copy link
Contributor

As RL was calling, I ran another AI game, this time only one error, with 2 warning messages on round 13 out of 17 rounds.

Is it worth adding the Round to the end of the WARN text in the triplea.log, to aid with debugging.

@asvitkine asvitkine removed the Major Indicates the problem is very impactful, very noticable, no work around label Jul 18, 2023
@TheDog-GH
Copy link
Contributor

TheDog-GH commented Jul 21, 2023

@asvitkine
So I dont appear to be able to reproduce the error, in a sandbox style.

Could you add the round & the name of the territory in the text output to the triplea.log ?

Here is an example from the triplea.log
95581 08:58:47.783 [AWT-EventQueue-0] WARN g.s.triplea.ui.BattleStepsPanel - Could not find step name:USSR select casualties

@asvitkine
Copy link
Contributor

asvitkine commented Aug 1, 2023

Marking as fixed again, via the additional fix here: #11838
This change should fix the more rare type of this issue that was still observed.

Please let me know if you still see this.

(Note: There is another type of "Could not find step name" error I saw on a different map that I plan to investigate, but I'm not sure if that one has anything to do with canNotTarget, so still closing this bug.)

@TheDog-GH
Copy link
Contributor

Darn, got to round 16 and got this error
4877329 09:04:09.405 [AWT-EventQueue-0] WARN g.s.triplea.ui.BattleStepsPanel - Could not find step name:Britain select casualties
4878173 09:04:10.249 [AWT-EventQueue-0] WARN g.s.triplea.ui.BattleStepsPanel - Could not find step name:Britain select casualties

Im happy for you to come back to this at a future date, as the error is now rare, compared to before.

@asvitkine
Copy link
Contributor

Thanks, can you post a save game before combat when it happens?

@TheDog-GH
Copy link
Contributor

Using TripleA-2.6+14460
Using 1941 GCD v130 https://drive.google.com/file/d/1KH0KTd7L1zKVDu_s22XcJ61kVlmIFzt0/view?usp=sharing

Error happened in Germany Round 2 from triplea.log
516609 08:45:29.688 [AWT-EventQueue-0] WARN g.s.triplea.ui.BattleStepsPanel - Could not find step name:Britain select casualties
516709 08:45:29.788 [AWT-EventQueue-0] WARN g.s.triplea.ui.BattleStepsPanel - Could not find step name:Britain select casualties

Zip has just before(round even) & during(Germany Combat Move)
autoSave.zip

@TheDog-GH
Copy link
Contributor

TheDog-GH commented Aug 5, 2023

@asvitkine
Black_Elk trapped this one
2023-8-5-1941-Global-Command-Decision v131 step casaulties error sz92 attack.zip

You might need v131
https://drive.google.com/file/d/18bzh2eGro5qGOq1MIdHwMkTo0HyeW0Ly/view?usp=drive_link

Click
Done
Air Battle in 092
Battle in 092

It errors 3 out 3 times, hopefully you can track it down?

@asvitkine asvitkine reopened this Aug 7, 2023
@asvitkine
Copy link
Contributor

Thanks, will take a look.

@asvitkine
Copy link
Contributor

As an aside, a lot of step names are generated for that battle:
Screenshot 2023-08-06 at 9 04 19 PM

So many that it doesn't even fit in the default window height and there's no scrollbar.

Anyway, here's what the log looks like for that battle:
Screenshot 2023-08-06 at 9 09 24 PM

So Britain loses both the fighter and bomber-tac.
So then, I guess after this happens, there's no longer separate groups of defenders for different attacks (since I guess some units could hit the air units and some couldn't). So the multiple combat steps get coalesced into one, without the UI getting an update.

@TheDog-GH
Copy link
Contributor

That looks like the issue, Black Elk also thought it was to do with the loss of aircraft.

Could the default window width be increased as some map makers use d12 and it looks very crowded with d12 on each side? In 2023 we have wide screens which were not common back in 2001.

If the step frame could be extended to the bottom, touching the space bar frame, then more steps can be seen?
Then the casualty frames would be pushed to the right, but because it wider it would still fit.

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
None yet
4 participants