Skip to content

Use composition mode for GameMode and Map#923

Draft
SadPencil wants to merge 2 commits intoCnCNet:developfrom
SadPencil:composition-gamemode-map
Draft

Use composition mode for GameMode and Map#923
SadPencil wants to merge 2 commits intoCnCNet:developfrom
SadPencil:composition-gamemode-map

Conversation

@SadPencil
Copy link
Member

@SadPencil SadPencil commented Feb 13, 2026

Previously on PR 719:

图片

@SadPencil
Copy link
Member Author

@Metadorius Is this what composition mode for? If so I think it is so dumb to copy-paste the boilerplate code twice

@SadPencil SadPencil requested a review from Metadorius February 13, 2026 08:15
@github-actions
Copy link

github-actions bot commented Feb 13, 2026

Nightly build for this pull request:

  • artifacts.zip
    This comment is automatic and is meant to allow guests to get latest automatic builds without registering. It is updated on every successful build.

@Metadorius
Copy link
Member

@Metadorius Is this what composition mode for? If so I think it is so dumb to copy-paste the boilerplate code twice

I didn't mean you should "forward" things like this. Just write this.CommonConfig.XYZ when you need to access something

@SadPencil
Copy link
Member Author

SadPencil commented Feb 13, 2026

@Metadorius Is this what composition mode for? If so I think it is so dumb to copy-paste the boilerplate code twice

I didn't mean you should "forward" things like this. Just write this.CommonConfig.XYZ when you need to access something

This is ugly because we have GameModeMap.MaxPlayers (returns int), GameMode.MaxPlayers (returns int?), and Map.MaxPlayers (returns int?). Now two of the threes becomes this.CommonConfig.XYZ while the rest one stays in this.XYZ

Even defining an interface for CommonConfig does not solve this issue since GameModeMap has a different nullablity on these properties

public int MaxPlayers =>
// Note: GameLobbyBase.GetMapList() assumes the priority.
// If you have modified the expression here, you should also update GameLobbyBase.GetMapList().
GameMode.MaxPlayersOverride ?? Map.MaxPlayers ?? GameMode.MaxPlayers ?? 0;
public int MinPlayers =>
GameMode.MinPlayersOverride ?? Map.MinPlayers ?? GameMode.MinPlayers ?? 0;
public bool MultiplayerOnly =>
Map.MultiplayerOnly ?? GameMode.MultiplayerOnly ?? false;

@Metadorius
Copy link
Member

This is ugly because we have GameModeMap.MaxPlayers (returns int), GameMode.MaxPlayers (returns int?), and Map.MaxPlayers (returns int?). Now two of the threes becomes this.CommonConfig.XYZ while the rest one stays in this.XYZ

Even defining an interface for CommonConfig does not solve this issue since GameModeMap has a different nullablity on these properties

public int MaxPlayers =>
// Note: GameLobbyBase.GetMapList() assumes the priority.
// If you have modified the expression here, you should also update GameLobbyBase.GetMapList().
GameMode.MaxPlayersOverride ?? Map.MaxPlayers ?? GameMode.MaxPlayers ?? 0;
public int MinPlayers =>
GameMode.MinPlayersOverride ?? Map.MinPlayers ?? GameMode.MinPlayers ?? 0;
public bool MultiplayerOnly =>
Map.MultiplayerOnly ?? GameMode.MultiplayerOnly ?? false;

solution: make a CommonConfig.Merge method that would return a merged config that you reference instead of individual things

@SadPencil
Copy link
Member Author

This is ugly because we have GameModeMap.MaxPlayers (returns int), GameMode.MaxPlayers (returns int?), and Map.MaxPlayers (returns int?). Now two of the threes becomes this.CommonConfig.XYZ while the rest one stays in this.XYZ

Even defining an interface for CommonConfig does not solve this issue since GameModeMap has a different nullablity on these properties

public int MaxPlayers =>
// Note: GameLobbyBase.GetMapList() assumes the priority.
// If you have modified the expression here, you should also update GameLobbyBase.GetMapList().
GameMode.MaxPlayersOverride ?? Map.MaxPlayers ?? GameMode.MaxPlayers ?? 0;
public int MinPlayers =>
GameMode.MinPlayersOverride ?? Map.MinPlayers ?? GameMode.MinPlayers ?? 0;
public bool MultiplayerOnly =>
Map.MultiplayerOnly ?? GameMode.MultiplayerOnly ?? false;

solution: make a CommonConfig.Merge method that would return a merged config that you reference instead of individual things

What do you mean? A CommonConfig and a MergedCommonConfig class? if not, how to make a different nullability?

@SadPencil
Copy link
Member Author

Based on previous discussion shown below, this PR is now paused due to the nullability issue. This composition pattern brings some heavy usages of ! operator and therefore is a heavy load to maintain and makes our codes (files with #nullable enable) less reliable against null reference exception.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants