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.

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

Persist skirmish settings between sessions #21206

Merged
merged 4 commits into from Nov 17, 2023
Merged

Conversation

pchote
Copy link
Member

@pchote pchote commented Nov 12, 2023

Closes #4122
Closes #4787

@pchote pchote force-pushed the lobby-options branch 3 times, most recently from 7a949cb to 55bb644 Compare November 12, 2023 18:16
@pchote
Copy link
Member Author

pchote commented Nov 12, 2023

Rewritten to split the skirmish logic from LobbyCommands into its own ServerTrait.

PunkPun

This comment was marked as resolved.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

IsSkirmish should be true when in lobby from the map editors PlayMap option

@pchote
Copy link
Member Author

pchote commented Nov 12, 2023

IsSkirmish should be true when in lobby from the map editors PlayMap option

No, as I mentioned above, this must be false to prevent the server logic from overriding the map being edited, and for the other state set in the editor from overriding the skirmish data.

@pchote

This comment was marked as resolved.

@RoosterDragon
Copy link
Member

RoosterDragon commented Nov 14, 2023

Overall seems good.

If I modify the saved file to create an invalid option (e.g. set gamespeed: no-such-option), then I can no longer open the Skirmish menu without an error
image

I don't think we need to protect against users messing with the file as such, but in the future if we change the lobby options or anything else that gets saved then the file on disk might not be valid any more. I think we need to recover from this scenario. Whether we drop the bad file entirly or try and recover individual options in a piecemeal fashion I don't mind.

@pchote
Copy link
Member Author

pchote commented Nov 16, 2023

Rebased and fixed.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@PunkPun PunkPun merged commit 89e1d71 into OpenRA:bleed Nov 17, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Nov 17, 2023

Changelog

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.

Remember faction for the mod? Remember lobby option checkbox states
3 participants