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

Fix game crashes when no maps are available to load #15565

Merged
merged 1 commit into from Sep 19, 2018

Conversation

Projects
None yet
6 participants
@noamyogev84
Copy link
Contributor

noamyogev84 commented Aug 26, 2018

Fixes #15492

Bug was reproduced on 2 scenarios:

  1. No maps available - Skirmish game started.
  2. No maps available - Map editor (load maps) opened.

The fix solves these 2 issues in a general way (by safely trying to iterate the maps (map previews) collection, and

  1. Skirmish - blocks server initializaion and prompts user on error.
  2. Map Editor - Opens the editor with no maps (only shell maps if available)
  • Detailed information in the commit message.

Thank you for your contribution to OpenRA!

Please be aware that we do not have enough project maintainers to match the rate of contributions, so it may take several days before somebody is able to respond to your Pull Request.

You can help speed up the review process by following a few steps:

  • Make sure that you have read and understand the OpenRA Coding Standard (see https://github.com/OpenRA/OpenRA/wiki/Coding-Standard).
  • Write quality commit messages (see https://chris.beams.io/posts/git-commit/).
  • Only commit changes that directly relate to your Pull Request. Use your Git interface to unstage any unrelated changes to project files, line endings, whitespace, or other files.
  • Review the code diff view below to double check that your changes satisfy the above three points.
  • Use the make test and make check commands to check for (and fix!) any issues that are reported by our automated tests.
  • If you are changing shared mod or engine code, make sure that you have tested your changes in all four default mods.
  • Respond to review comments as soon as you reasonably can. Reviewers will usually prioritize Pull Requests that are still fresh in their minds. Make sure to leave a comment when you push new changes, otherwise GitHub does not automatically notify reviewers!
  • Leave a polite comment asking for reviews if a week or more has passed without feedback.

If you need any help you can ask in the #openra IRC channel on freenode (most active during European evenings).

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Aug 29, 2018

Took a glimpse on the committed code - while I like your intention to throw a specific crash, such intense special error casing is unneeded IMO.

Although I'm notsure suddenly where the error message would appear ultimately, which might justify your ideals here. If TryWithPrompt is valid though, UtilityCommands is still a bad place for it - that's specific area for the OpenRA.Utility and not something which should be leaked back to our widget codebase IMO.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 29, 2018

I agree with @GraionDilach: an InvalidDataException would probably do just as well here.

IMO the prompt would be better replaced by disabling the skirmish and create server buttons if no maps are available. IIRC we already do this for the Missions button if the mod doesn't define any missions.

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Aug 29, 2018

Thanks for the review.
I'm a bit confused on how to re tackle this based on your review. @GraionDilach - changing the specific exception and moving TryWithPrompt to a better place is enough? or should I take @pchote approach and don't block to the skirmish view? (if I understand correctly)

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 29, 2018

To clarify my point above, this is what happens when no missions are available (greyed out means it can't be clicked):

screen shot 2018-08-29 at 20 10 02

It is controlled by this code:

var missionsButton = singleplayerMenu.Get<ButtonWidget>("MISSIONS_BUTTON");
missionsButton.OnClick = OpenMissionBrowserPanel;
var hasCampaign = modData.Manifest.Missions.Any();
var hasMissions = modData.MapCache
.Any(p => p.Status == MapStatus.Available && p.Visibility.HasFlag(MapVisibility.MissionSelector));
missionsButton.Disabled = !hasCampaign && !hasMissions;

You can do the same thing for Skirmish by doing something very similar to the line immediately below this block in MainMenuLogic, and on the Create button in MultiplayerLogic.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Aug 29, 2018

Definitely opting for this latter approach with explicitly removing all code changes unnecessary to it (including the TryWithPrompt helper).

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Aug 29, 2018

OK. starting to work on it right away.
Ofcourse I'll try to do the same for the Load Map button as well.

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Aug 31, 2018

@GraionDilach @pchote
Hi guys,
Please have a quick look, this is much simpler than my first approach, and aligns with your suggestions.

One concern I have - shouldn't we track/log the reason for this behavior? How does one knows why those buttons are disabled? seems a bit unclear.

@@ -332,8 +332,8 @@ public string ChooseInitialMap(string initialUid, MersenneTwister random)
if (string.IsNullOrEmpty(initialUid) || previews[initialUid].Status != MapStatus.Available)
{
var selected = previews.Values.Where(IsSuitableInitialMap).RandomOrDefault(random) ??
previews.Values.First(m => m.Status == MapStatus.Available && m.Visibility.HasFlag(MapVisibility.Lobby));
return selected.Uid;
previews.Values.FirstOrDefault(m => m.Status == MapStatus.Available && m.Visibility.HasFlag(MapVisibility.Lobby));

This comment has been minimized.

@reaperrr

reaperrr Sep 1, 2018

Contributor

Please replace the spaces with a tab like on the original line.

This comment has been minimized.

@noamyogev84

noamyogev84 Sep 1, 2018

Contributor

done 👍

@@ -122,7 +122,10 @@ public MainMenuLogic(Widget widget, World world, ModData modData)

missionsButton.Disabled = !hasCampaign && !hasMissions;

singleplayerMenu.Get<ButtonWidget>("SKIRMISH_BUTTON").OnClick = StartSkirmishGame;
var hasMaps = modData.MapCache.Any(p => !p.Visibility.HasFlag(MapVisibility.Shellmap));

This comment has been minimized.

@pchote

pchote Sep 1, 2018

Member

These should instead be checking .Any(p => p.Visibility.HasFlag(MapVisibility.Lobby)), which is the criteria used in the map chooser.

This comment has been minimized.

@noamyogev84

noamyogev84 Sep 1, 2018

Contributor

done 👍

@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@pchote

pchote Sep 1, 2018

Member

Please unstage this.

This comment has been minimized.

@noamyogev84

noamyogev84 Sep 1, 2018

Contributor

Not sure where it's coming from...
I see no changes between my first commit and the ones after...

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 10, 2018

Member

Weird that this still appears. Maybe it goes away when you squash the commits together.

This comment has been minimized.

@noamyogev84

noamyogev84 Sep 11, 2018

Contributor

@abcdefg30 would you care to verify the squashed commits? :)

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 11, 2018

Member

The squashing worked, yes. It'd be nice if you could fix the commit message, however. (Just use the message of the first commit but nothing more). Unfortunately, the change here didn't go away, so please try unstaging/removing it again. (Don't hesitate to ask if you need help with this.)

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 11, 2018

Member

I don't know which git client you use, so here is the git-bash version.

git reset --soft head~1
git reset OpenRA.Game/OpenRA.Game.csproj
git checkout OpenRA.Game/OpenRA.Game.csproj

The first command deletes your commit and moves the files back into the staging area.
The second command moves this file out of the staging area, so the third command can remove the change.
After that is done, just re-commit your changes and push again.
Hope that helps. :)

This comment has been minimized.

@noamyogev84

noamyogev84 Sep 13, 2018

Contributor

Hey @abcdefg30 . how is it looking? had more trouble with git than the actual fix... are we good now?

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 14, 2018

Member

Sorry, something must have gone wrong, the change is still there. To make things worse, some unwanted commits sneaked in which means you will have to rebase again.
Yes, git can be hard to work with... You might want to join our IRC channel for direct help. (Unfortunately, the channel is restricted to registered accounts currently...)
Worst case one of us (probably me ^^) could do the nasty git work for you, if you wish. (I assume you have "Allow edits from maintainers" enabled for this PR.)

This comment has been minimized.

@noamyogev84

noamyogev84 Sep 14, 2018

Contributor

@abcdefg30 sure go ahead.

This comment has been minimized.

@abcdefg30
@Smittytron

This comment has been minimized.

Copy link
Contributor

Smittytron commented Sep 8, 2018

Please squash commits

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Sep 10, 2018

@pchote @reaperrr @GraionDilach Hey guys, is there anything wrong with the fix? pinging you as it's been a while....

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 10, 2018

Sorry @noamyogev84, i've been travelling for work for the last couple of weeks, and won't be able to do any OpenRA reviewing before next weekend.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me otherwise.

@@ -332,8 +332,8 @@ public string ChooseInitialMap(string initialUid, MersenneTwister random)
if (string.IsNullOrEmpty(initialUid) || previews[initialUid].Status != MapStatus.Available)
{
var selected = previews.Values.Where(IsSuitableInitialMap).RandomOrDefault(random) ??
previews.Values.First(m => m.Status == MapStatus.Available && m.Visibility.HasFlag(MapVisibility.Lobby));
return selected.Uid;
previews.Values.FirstOrDefault(m => m.Status == MapStatus.Available && m.Visibility.HasFlag(MapVisibility.Lobby));

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 10, 2018

Member

This looks like you are introducing one level of indentation too much.

@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 10, 2018

Member

Weird that this still appears. Maybe it goes away when you squash the commits together.

@noamyogev84 noamyogev84 force-pushed the noamyogev84:fix_issue_15492 branch 2 times, most recently from 7025293 to 21142c6 Sep 11, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Sep 14, 2018

Please don't ping me, I've fed up with this project.

add NoAvailableMaps exception.
modify ChooseInitialMap to throw NoAvailalbeMaps exception if no maps were loaded.
implement Utilities.TryWithPrompt - safe execution of a provided action with ability to prompt user on error.

@abcdefg30 abcdefg30 force-pushed the noamyogev84:fix_issue_15492 branch from 2509cc4 to 313b2c6 Sep 14, 2018

@Smittytron
Copy link
Contributor

Smittytron left a comment

Works

@abcdefg30 abcdefg30 merged commit 4e7a35b into OpenRA:bleed Sep 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 19, 2018

@noamyogev84 noamyogev84 deleted the noamyogev84:fix_issue_15492 branch Sep 20, 2018

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