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 crash when adding a player with a removed owner selected #15628

Merged
merged 1 commit into from Oct 7, 2018

Conversation

Projects
None yet
4 participants
@noamyogev84
Copy link
Contributor

noamyogev84 commented Sep 20, 2018

fix #9364
fix #15623
fix #14924

added an event/event handler to EditorLayer/PlayerSelectorLogic to notify when selected owner should be updated.
Detailed information in commit message.

@noamyogev84 noamyogev84 changed the title Fix game crash when adding player with a removed owner selected Fix game crash when adding a player with a removed owner selected Sep 20, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 20, 2018

I suspect this may overlap with #15059, but I still don't have the time to spare to look at either PR in detail again yet.

editorLayer.PlayerRemoved -= EditorLayer_PlayerRemoved;
}

void EditorLayer_PlayerRemoved(object sender, PlayerRemovedEventArgs e)

This comment has been minimized.

@Mailaender

Mailaender Sep 20, 2018

Member

The underscore notation looks a bit alien here.

This comment has been minimized.

@noamyogev84

noamyogev84 Sep 20, 2018

Contributor

can fix, no probs!


void EditorLayer_PlayerRemoved(object sender, PlayerRemovedEventArgs e)
{
if (!selectedOwner.Name.Equals(e.RemovedPlayerName)) return;

This comment has been minimized.

@Mailaender

Mailaender Sep 20, 2018

Member

Same for this inline return.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 20, 2018

Please lets be careful about the potential PR duplication before this goes further - I would like to avoid another #15493 / #15588 situation.

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Sep 20, 2018

Ok. waiting for your decision on this.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 1, 2018

I have a bit more time to be able to spend on OpenRA again, and have looked at this vs #15059 in more detail. Sorry for the delay.

I think #15059 is a bit more in line with our coding conventions (no need to pull in EventArgs etc here), but @TamasKiss has been inactive for quite some time and that PR also requires changes. If you don't mind picking this up again, I think the best way forward will be for you to take a look at #15059 and its review comments, then update this one as a hybrid of the current two.

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Oct 2, 2018

@pchote ok :)

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Oct 3, 2018

@pchote feels all the upstream/origin gaps leeks into this last push i did. anyway. I did align the fix according to #15059 implementation, also removed the part that tries to fallback to some other 'multi_x' player, for me it seems like the best option is to just fallback to Neutral. please tell me what you think.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 3, 2018

It looks like you did a merge rather than a rebase. Take a look at https://github.com/OpenRA/OpenRA/wiki/Contributing#keeping-up-to-date-rebasing for tips on how to fix this

@noamyogev84 noamyogev84 force-pushed the noamyogev84:fix_issue_15623 branch from 6b4b0e3 to c97615d Oct 3, 2018

@noamyogev84 noamyogev84 closed this Oct 3, 2018

@noamyogev84 noamyogev84 force-pushed the noamyogev84:fix_issue_15623 branch from c97615d to a7de079 Oct 3, 2018

@noamyogev84 noamyogev84 deleted the noamyogev84:fix_issue_15623 branch Oct 3, 2018

@noamyogev84 noamyogev84 restored the noamyogev84:fix_issue_15623 branch Oct 3, 2018

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Oct 3, 2018

@pchote had to reset my work due to an error I can't shake. can you please reopen the PR?

@pchote pchote reopened this Oct 3, 2018

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Oct 6, 2018

Hi @pchote. Pinging you to see if there's any more work need to be done here?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 6, 2018

No, this looks good to me now. Thanks!

@pchote pchote added the PR: Needs +2 label Oct 6, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me otherwise. 👍

{
if (editorLayer.Players.Players.Values.Any(p => p.Name == selectedOwner.Name))
return;
SelectOwner(editorLayer.Players.Players.Values.FirstOrDefault());

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 6, 2018

Member

I think this can get .First(). We should always have at least one player anyway. (And this would crash later on as well if we passed null here.)

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 6, 2018

Uh oh, it looks like you've merged in bleed commits again! Make sure to never use git pull (unless you reconfigure it to use rebase instead of merge)!

If you'd like me or @abcdefg30 can push over the branch to fix this for you.

@noamyogev84

This comment has been minimized.

Copy link
Contributor

noamyogev84 commented Oct 7, 2018

@pchote @abcdefg30 No problems, Thanks!
This is the second time that it happens, rebasing didn't help for some reason.

@pchote pchote force-pushed the noamyogev84:fix_issue_15623 branch from c2e485f to bfef404 Oct 7, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 7, 2018

Fixed. I also squashed the commits together and gave them a more recognizable commit message.

@pchote

pchote approved these changes Oct 7, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 7, 2018

Ping @abcdefg30 to confirm your 👍

@abcdefg30 abcdefg30 merged commit c71f97e into OpenRA:bleed Oct 7, 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 Oct 7, 2018

Changelog

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