-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor(GameState): extract common code from MainMenu and HeadlessSetup #4809
Conversation
// TODO: Reduce coupling between Input system and CameraTargetSystem, | ||
// TODO: potentially eliminating the following lines. See Issue #1126 | ||
CameraTargetSystem cameraTargetSystem = new CameraTargetSystem(); | ||
context.put(CameraTargetSystem.class, cameraTargetSystem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I've removed this use of CameraTargetSystem in the main menu, but that's not really the case. It still happens within RegisterInputSystem called in this method.
protected EventSystem eventSystem; | ||
protected ComponentSystemManager componentSystemManager; | ||
|
||
protected void initEntityAndComponentManagers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does a lot with the context
and strictly requires it. Would it make sense to make the context
and argument of this method, similar to how we pass it to createLocalPlayer
explcitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createLocalPlayer
is the method I extracted first. Passing context
let me make it static
, which I figured would make it easier to move the method later if there were more copies of it.
Then I realized if I shuffled some things around I could also extract all this stuff that went in to initEntityAndComponentManagers
, but because it assigns things to multiple instance fields, it wouldn't work as static.
Upon review, I see that all these fields are direct references to a thing in the context, so it probably could be split in to a "populate context
with this stuff" static method and then a "pull some references out of context and cache them in fields" non-static method, but I worried that would make it more difficult to review if I included it in the same PR.
I was working with the StateHeadlessSetup class and noticing it has a lot of code duplicated from another class. To make it easier to see what the differences between the various classes are, I extracted the common code to some methods.
No change in functionality is intended by this PR, but I did re-order the execution of some of these components, which has the potential to break something.
Notes to Reviewers
I'm not thrilled about the state of things here, but it's a refactor, so please try to limit the scope of your requests to the refactoring. I'd also like to avoid getting pulled down the rabbit hole of putting all the potentially related refactors we can think of into this PR.
Things it would be useful to have feedback on:
AbstractState
is a particularly unhelpful name.