-
Notifications
You must be signed in to change notification settings - Fork 26
132 component event framework #277
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
base: Development
Are you sure you want to change the base?
Conversation
Move ComponentManager to the C7Engine project and namespace. Implement the CalendarComponent responsible for updating turn number and date. For now it is triggered by an event from TurnHandling and logs the date to console.
…efactor game scene and init sequence to better manage data and callbacks.
Moved ComponentManager and components into new folder and namespace, gave GameComponent an Initialize method, and allowed chaining ComponentManager method calls
Created a stub implementation for AutosaveComponent, reacting to TurnStarted events by generating an autosave filename
Added a stubbed-out AutosaveComponent, so you can start to see how the components can loosely interact with each other. ComponentManager is now a little more manage-able with method chaining, since presumably all (default) components will be added and initialized in one go. |
@QuintillusCFC and @maxpetul is this helpful so far? I want to at least refine the Node integration before merging but hopefully this gives you an idea of how the components can work together. There's not much in the way of events yet since these don't do much. I was thinking about refactoring unit combat into a component since there's a lot more going on there, but after looking at it I'm not sure how much it should be component-ized. It kind of makes sense for unit objects to act on each other directly, unlike other more abstract mechanics like governments or research. |
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 definitely helps illustrate the event/component paradigm as it relates to C7 for me. (I'm also bad at going out and reading documentation unless I need it right now, and learn best by example) I can definitely see how this lends itself to extensibility, and the calendar is a nice example. Now I know why you suggested a RandomComponent in the other PR.
// END Production phase | ||
|
||
gameData.turn++; | ||
TurnEnded?.Invoke(null, null); |
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.
Is there a reason the sender
argument is null here, but this
in the other event invocations?
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
// modeled after the Unity Toolbox pattern from https://wiki.unity3d.com/index.php/Toolbox |
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 the Unity 3D Wiki moved, or maybe is down; when I try to go to that site I get wiki.unity3d.com’s server IP address could not be found.
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.
Not sure what the original tutorial was but this looks somewhat related https://learn.unity.com/project/65de084fedbc2a0699d68bfb
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.
Here's the archive of the wiki page: https://web.archive.org/web/20210304075110/https://wiki.unity3d.com/index.php/Toolbox
It's really just the concept of having a dumb Singleton container to hold unique instances of classes instead of making them all Singletons themselves. There may be a more Godot canonical way of achieving that part.
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.
Let me clarify, the Unity and Godot references are irrelevant here; this is pure C# in the engine layer. Even the Unity implementation was derived from the original source on an IBM blog, now mirrored here: https://gurkantech.blogspot.com/2008/10/use-your-singletons-wisely_02.html
The core idea here is that being a Singleton has messy implications, so you make the Singleton itself as simple as possible and have it keep references to other arbitrary objects that you want to make globally accessible. I've seen similar implementations elsewhere: for example, Android's Context.getSystemService takes a class or name and returns the corresponding "service" object which is an arbitrarily typed resource that is unique to some scope but not a Singleton.
_gameData.turn = nextTurnNumber; | ||
CurrentTurn = GetGameTurn(nextTurnNumber); | ||
|
||
Console.WriteLine("Date is now " + CurrentTurn.TurnDate); |
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.
Probably ought to switch these to Serilogs now that we have that in the engine.
date = String.Concat(date.Where(c => !char.IsWhiteSpace(c))); | ||
date = String.Join("_", date.Split(Path.GetInvalidFileNameChars(), StringSplitOptions.RemoveEmptyEntries)); | ||
string filename = $"auto_{date}.json"; | ||
Console.WriteLine($"I would save {filename} right now if I knew how..."); |
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.
In part that's part of card #223 , in part it would use SaveFormat.Save in the C7GameData project. But I could also see having a component for saving/loading (not just autosaving...), and that's really slightly beyond this proof-of-concept.
(Also slightly beyond, but it occurred to me that this method, or at least the file name creation part of it, would lend itself nicely to some self-documenting units tests)
{ | ||
//Oh hai, we do need this handler here! | ||
LowerRightInfoBox.SetTurn(turnNumber); | ||
//LowerRightInfoBox.SetTurn(turnNumber); |
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.
Probably can take this method out now, I think... or maybe we should leave it for now since there are other things that will need updated as we add more features?
All the previous comments are stylistic/not functional issues. Now I've tested it in-game, and noticed a couple things:
Architecturally, the one caveat I'd note is that the front-end C7 project, specifically GameStatus.cs, is listening directly to the TurnStarted event. This is fine in single player, but I think it's a good example of how multi-player is tougher. Maybe there's still a TurnStarted event in MP that gets invoked in a different path? There are plenty of MP-related problems with the old way of doing things too. I'm definitely not saying it needs to be changed, it's more "if you/someone else on the team really wants MP, this might be the time to figure out how it would work". |
Here is a slightly more involved example that's been kicking around in my head for a long time. I wrote it on the fly in a text editor and it assumes some features that don't yet exist, so consider it pseudocode but it illustrates how some actual game logic can be decoupled from the data it's working with:
Notice a few things here:
|
More notes... Currently the ComponentManager registers components by their actual type. Consider either giving AddComponent an override to take an explicit key type OR giving GameComponent a function to return the desired key type which defaults to its own type. This would allow mock types to imitate their parent or other types in tests so as to be transparent to other components that depend on them. Would this be a code smell, and if so does it suggest that Singleton is the wrong tactic here (since the Singleton cannot practically be extended to modify its behavior for tests) and the ComponentManager should instead be accessible by widespread dependency injection? GameComponent should have an Init() method, which ComponentManager calls after all components have been added. This allows for a two-step initialization so that components can arbitrarily depend on each other regardless of the order they are created in. More GameComponent candidates:
|
This will resolve #132
There's still some work to do especially for events but I wanted to get this much up for reference. I've implemented one of the simplest components, the calendar: it just updates the game turn and calculates the corresponding date.
Moved ComponentManager to C7Engine as it is part of the core code, and implemented CalendarComponent. Added component initialization in CreateGame, and replaced the turn increment in TurnHandling with an event for the calendar to subscribe to. Added a function in GameStatus to connect the component's event to the info box update. and call it just before the game starts.
The latter has to be done outside of _Ready for now because of the order of events in Godot: a node isn't ready until after all its children are ready, so _Ready propagates up the tree and thus Game initialization has not happened yet when UI elements become ready. I tried moving some code from Game._Ready to Game._EnterTree but immediately ran into problems so I went with a follow-up call into the child node. This is a common pattern as parents should be responsible for configuring their children, but I'm not necessarily happy with this solution in particular. That is probably a larger architectural issue to think about game-wide.
You'll also notice that I passed the game data reference into the component constructor even though it's a static object. We should avoid relying on static data (both dependency injection and the component model will help with that when used carefully), because it makes it very hard to track state and to test classes in isolation. That's another thing to address at a higher level, but here's a small counter-example.
So this is all just a first cut of everything, especially the Godot portion, but shows how the components can interact with the rest of the code. I still want to show more events, and maybe a second component like autosave so they can interact with each other.