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

Put game on diet #19360

Closed
wants to merge 4 commits into from
Closed

Put game on diet #19360

wants to merge 4 commits into from

Conversation

teinarss
Copy link
Contributor

Moving some of the code from the Game class to dedicated classes.

OpenRA.Game/HotkeyModifiers.cs Show resolved Hide resolved
OpenRA.Game/Exts.cs Show resolved Hide resolved
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

Thinking out loud, not necessarily review requests. I expect that some/all of these would create nasty plumbing issues due to inaccesible state.


namespace OpenRA
{
public class ExternalModSwitcher
Copy link
Member

Choose a reason for hiding this comment

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

Can this be merged into the ExternalMods class and then called as Game.ExternalMods.Switch()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about that too, was not sure about that so started with the dedicated class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is that we need to supply launchWrapper , we could supply it through the constructor, but we use ExternalMods in other places where we dont have that and also not using it.

@@ -0,0 +1,56 @@
#region Copyright & License Information
/*
* Copyright 2007-2020 The OpenRA Developers (see AUTHORS)
Copy link
Member

Choose a reason for hiding this comment

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

2021

OpenRA.Game/HotkeyModifiers.cs Show resolved Hide resolved
{
public class ScreenshotManager
{
public static void TakeScreenshot()
Copy link
Member

Choose a reason for hiding this comment

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

This would probably make sense merged into Renderer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another idea is to move most of the logic from the screenshot logic code out from the renderer class and only let it return the data.


namespace OpenRA
{
public class TextManager
Copy link
Member

Choose a reason for hiding this comment

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

Is there any sensible owner (e.g. ModData) that would allow this to not be static?

@@ -0,0 +1,36 @@
#region Copyright & License Information
/*
* Copyright 2007-2020 The OpenRA Developers (see AUTHORS)
Copy link
Contributor

@reaperrr reaperrr Apr 20, 2021

Choose a reason for hiding this comment

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

Likewise

@PunkPun
Copy link
Member

PunkPun commented Nov 25, 2022

Do we want to keep this PR open? It seems we've already done many of the proposed changes and the name of the PR is fairly missleading

@PunkPun PunkPun closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants