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.

Sign up for GitHub

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

Jump to bottom

Fix crash on empty map #456

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Fix crash on empty map #456

merged 2 commits into from
Sep 23, 2024
+40 −34

Conversation

SadPencil
Copy link
Member

@SadPencil SadPencil commented Apr 2, 2023 •
edited
Loading

Previously, if a player does not select a map, it will either make the game room invisible (for CnCNet) or causes the client to crash (for LAN). Changing a game option when the map is deselected also causes the client to crash.

Related codes have been reworked, so that:

  • A game room whose map is deselected is correctly shown in the list, instead of missing or a crash.
  • The change will be correctly updated to all players when the map is deselected and the host changes a game option, instead of a crash.
  • The change will be correctly updated to all players when the host deselect a map, instead of a crash.

Snipaste_2024-09-23_00-07-47
Snipaste_2024-09-23_00-08-06

@SadPencil
Copy link
Member Author

SadPencil commented Apr 2, 2023 •
edited
Loading

Workaround: when Map or GameMode is null, don't send broadcast message.
It is okay? @devo1929

@github-actions
Copy link

github-actions bot commented Apr 2, 2023 •
edited
Loading

Nightly build for this pull request:

@SadPencil
Copy link
Member Author

After investigating this issue again, it seems this is another eight-year-old bug.

https://github.com/CnCNet/xna-cncnet-client/blame/74ca2f2657e0bd55f46f86a4d3a95f0d7b60665a/DXMainClient/DXGUI/Multiplayer/GameLobby/LANGameLobby.cs#L745

Hopefully this PR fixes it. Needs to be tested.

@SadPencil
Copy link
Member Author

SadPencil commented Sep 19, 2024 •
edited
Loading

When Map or GameMode is null, the client now sends "Unknown" as their names instead of crashing

@SadPencil SadPencil changed the title Map and GameMode should not be null Fix LAN and multi-player crash on empty map or game mode Sep 19, 2024
@SadPencil SadPencil marked this pull request as ready for review September 22, 2024 12:25
@SadPencil
Copy link
Member Author

Note: apart from an empty favorite map list, one can also trigger this crash by typing random words using the search box. As long as the map list is not full, one can right-click the background in the map list to select null as the map.

@SadPencil SadPencil added this to the 2.11.1.0 milestone Sep 22, 2024
@SadPencil SadPencil changed the title Fix LAN and multi-player crash on empty map or game mode Fix LAN crash on empty map or game mode Sep 22, 2024
Metadorius
Metadorius reviewed Sep 22, 2024
View reviewed changes
DXMainClient/Domain/Multiplayer/MapLoader.cs Outdated
@@ -114,6 +117,8 @@ private void LoadMultiMaps(IniFile mpMapsIni)
maps.Add(map);
}

_translatedMapNames["Unknown"] = "Unknown".L10N("Client:Main:UnknownMap");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the game mode? I would also just leave it at "Client:Main:Unknown" so that it is shared.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it's covered in CnCNetLobby.cs file:

            string translatedGameMode = hg.GameMode.L10N($"INI:GameModes:{hg.GameMode}:UIName", notify: false);

I can also write an if statement here if you think "Client:Main:Unknown" is better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it fail to parse the game mode before that though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right let me think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think it's a special case, so it needs special handling. Notification for the translation system for sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it fail to parse the game mode before that though?

hg.GameMode is a string

@SadPencil SadPencil marked this pull request as draft September 22, 2024 14:17
@SadPencil SadPencil changed the title Fix LAN crash on empty map or game mode Fix crash on empty map Sep 22, 2024
@SadPencil SadPencil marked this pull request as ready for review September 22, 2024 16:17
@SadPencil
Copy link
Member Author

SadPencil commented Sep 22, 2024 •
edited
Loading

de08ac4#diff-777cb9df7b1707381d10997b31f560267ea801103b512c289c26765fa4454623R1367

8 years ago, CnCNet crash on null map was fixed, but LAN crash was left as is.

Metadorius
Metadorius reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@Metadorius Metadorius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from the changes proposed

DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs Outdated Show resolved Hide resolved
DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs Outdated Show resolved Hide resolved
DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs Show resolved Hide resolved
DXMainClient/DXGUI/Multiplayer/GameInformationPanel.cs Outdated Show resolved Hide resolved
DXMainClient/DXGUI/Multiplayer/GameInformationPanel.cs Outdated Show resolved Hide resolved
@SadPencil @Metadorius
Apply code style changes
3dfb059 
Co-authored-by: Kerbiter <crabiter@vivaldi.net>
@Metadorius Metadorius merged commit 3c1ccd3 into CnCNet:develop Sep 23, 2024
3 checks passed
@SadPencil SadPencil deleted the fix-empty-fav-list branch September 23, 2024 17:59
SadPencil added a commit to SadPencil/xna-cncnet-client that referenced this pull request Oct 4, 2024
@SadPencil @Metadorius
Fix crash on empty map (CnCNet#456)
3c56b69 
* Fix crash on empty map

* Apply code style changes

Co-authored-by: Kerbiter <crabiter@vivaldi.net>

---------

Co-authored-by: Kerbiter <crabiter@vivaldi.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Metadorius Metadorius Metadorius approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.11.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@SadPencil @Metadorius