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

Connection Failed when trying to join custom map #12881 #12976

Merged
merged 1 commit into from Apr 11, 2017

Conversation

Projects
None yet
4 participants
@rob-v
Contributor

rob-v commented Mar 16, 2017

ToDictionary(o => o.Id, o => o) failed due to duplicated Rule Id, e.g. PlayerResourcesInfo rules from Player and World, but it seems options from server.Map.Rules aren't needed here, as they are all added in LoadMapSettings function into defaults so I assume it is better to simply remove it than fix the duplicity again here.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Mar 17, 2017

Member

Be very careful here. The logic for propagating rule settings when changing map or player joining is subtle, and I expect that removing this will regress something (but I don't have the time to refamiliarize myself with the code to tell you specifically what).

Member

pchote commented Mar 17, 2017

Be very careful here. The logic for propagating rule settings when changing map or player joining is subtle, and I expect that removing this will regress something (but I don't have the time to refamiliarize myself with the code to tell you specifically what).

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Mar 18, 2017

Contributor

Ok. Here is picture that could help you save some time:

...

I think it can be removed, but if you prefer, I propose to change Dictionary to List, as it only checks key's (option name) presence not value.

...

It works with List as it can contain duplicate Id (option name) and the connection doesn fail then for some maps.

Contributor

rob-v commented Mar 18, 2017

Ok. Here is picture that could help you save some time:

...

I think it can be removed, but if you prefer, I propose to change Dictionary to List, as it only checks key's (option name) presence not value.

...

It works with List as it can contain duplicate Id (option name) and the connection doesn fail then for some maps.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Mar 18, 2017

Member

I'm sorry to say that the regression here isn't even subtle: this makes the server print the internal id of the options into the chat instead of the human readable name/label.

Member

pchote commented Mar 18, 2017

I'm sorry to say that the regression here isn't even subtle: this makes the server print the internal id of the options into the chat instead of the human readable name/label.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Mar 18, 2017

Contributor

yes, sorry. I was thinking about that sent key, but unfortunately didn't verify this one. Please check last commit.

Contributor

rob-v commented Mar 18, 2017

yes, sorry. I was thinking about that sent key, but unfortunately didn't verify this one. Please check last commit.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 2, 2017

Contributor

It was split into 2 operations as requested. It is basically now your solution, so you could create your PR to be merged.

Contributor

rob-v commented Apr 2, 2017

It was split into 2 operations as requested. It is basically now your solution, so you could create your PR to be merged.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 2, 2017

Member

Thanks for the fixes. Code looks good to me now, but I haven't tested it yet.

Member

pchote commented Apr 2, 2017

Thanks for the fixes. Code looks good to me now, but I haven't tested it yet.

@obrakmann

The code looks sensible, and the map in question doesn't crash anymore.

However, this does come down to us accepting maps with broken rules, which I'm not sure I'm ok with.

Our stance has always been that it's better to crash than to silently ignore errors.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 8, 2017

Member

Ideally we would not crash but generate a lint error.

Member

pchote commented Apr 8, 2017

Ideally we would not crash but generate a lint error.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 8, 2017

Contributor

Is it broken map? This map works normally, some rule is overriden. In core places it is accepted, the only place when it fails is minor/optional - it just informs joined client about changed settings in chat.

Contributor

rob-v commented Apr 8, 2017

Is it broken map? This map works normally, some rule is overriden. In core places it is accepted, the only place when it fails is minor/optional - it just informs joined client about changed settings in chat.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 8, 2017

Member

19:28 <+pchote> antares79: what are those maps doing wrong?
19:30 <+antares79> they have the PlayerResources trait on the world actor

Member

pchote commented Apr 8, 2017

19:28 <+pchote> antares79: what are those maps doing wrong?
19:30 <+antares79> they have the PlayerResources trait on the world actor

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 8, 2017

Contributor

There is also another issue with map failure only in MP due to this I assume. We can inhance lint checks, but this new client joined info has different logic than core logic.
Isn't there a valid case when a rule can override another rule? If yes, then this case has to be still fixed.

Contributor

rob-v commented Apr 8, 2017

There is also another issue with map failure only in MP due to this I assume. We can inhance lint checks, but this new client joined info has different logic than core logic.
Isn't there a valid case when a rule can override another rule? If yes, then this case has to be still fixed.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 8, 2017

Member

I agree that this should still be fixed, but mainly because we already handle duplicates in all cases but this.

The crash is caused because the map is incorrectly putting the trait on a different actor, which means that there are legitimately multiple sources of that map setting. An actual override will not trigger that crash.

Member

pchote commented Apr 8, 2017

I agree that this should still be fixed, but mainly because we already handle duplicates in all cases but this.

The crash is caused because the map is incorrectly putting the trait on a different actor, which means that there are legitimately multiple sources of that map setting. An actual override will not trigger that crash.

@pchote

pchote approved these changes Apr 10, 2017

This brings the notification behaviour in line with the parsing behaviour, so 👍 from me.

@pchote pchote added the PR: Needs +2 label Apr 10, 2017

@abcdefg30 abcdefg30 merged commit 3808a18 into OpenRA:bleed Apr 11, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Apr 11, 2017

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