First draft of adding IOC container #1841
Conversation
|
|
||
| public interface IGameBuilder | ||
| { | ||
| public ConfigurationManager Configuration { get; } |
There was a problem hiding this comment.
ConfigurationManager standardizes application settings in one place.
They can come from a file, command line argument, environment variable, etc.
They get combined and overwrite each other. For example, ASP default is
to have appsettings.json, the values of which can be overwritten in debug
or release mode, by having appsettings.Development.json or
appsettings.Production.json respectively.
We can set up a sensible default order for these sources, but out of the box
this ConfigurationManager allows any game developer to override it as they please.
A YAML configuration provider does not exist AFAIK, but should be trivial to implement by deriving from FileConfigurationProvider
https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.configuration.fileconfigurationprovider?view=dotnet-plat-ext-7.0
Another cool thing is that one possible source is "User Secrets"
https://learn.microsoft.com/en-us/aspnet/core/security/app-secrets?view=aspnetcore-7.0&tabs=windows
Which allows a game developer team member to override any configuration value
without touching anything in the project folder.
It would also be trivial to add dotnet's code for what they call "the options pattern"
https://learn.microsoft.com/en-us/dotnet/core/extensions/options
which allows an injected class to choose between getting a configuration object that
A) Just has the options from when the app started
B) Has updated values if the file was changed after app start
C) Allows to register a listener for when the file changes
Don't know if there's use for that in game dev except debugging, though.
There was a problem hiding this comment.
Similar to my comment below, we'd probably want them (and the engine) constructing configuration through a series of configuration actions created as a collection via the .ConfigureHostConfiguration() call or similar. This could store as a List<Action<ConfigurationBuilder>> or List<Action<HostBuilderContext, ConfigurationBuilder>>.
While building the Game instance, an IConfiguration would be built using these actions instead of relying on ConfigurationManager at runtime (which may make File I/O calls). The code below demonstrates how that could be built:
var configBuilder = new ConfigurationBuilder(); // Other extensions applied.
foreach (Action<HostBuilderContext, IConfigurationBuilder> buildAction in _configureAppConfigActions)
{
buildAction(_hostBuilderContext, configBuilder);
}
_appConfiguration = configBuilder.Build();I haven't personally used ConfigurationManager much, so I may be off base.
There was a problem hiding this comment.
IIRC at least in Sdk.Web you can do it either way, and one way or the other isn't really more idiomatic than the other. So I guess it kinda comes down to the choice of also giving the users both options, or making a judgement call what to support.
Also, just in general - as a comment that's also an answer to other things further down: In .NET 8, the generic host was made kinda... more "accessible" (?) - to the point where in my own project, switching to the standard IHost allowed me to get rid of some homebrew stuff in favour of the Microsoft-provided standard.
If that's also a good fit for Stride - eh, you know it far better than me :D
| public interface IGameBuilder | ||
| { | ||
| public ConfigurationManager Configuration { get; } | ||
| public IServiceCollection Services { get; } |
There was a problem hiding this comment.
IServiceCollection standardizes registering dependencies.
It comes with many, many extension methods for convenience.
https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.iservicecollection?view=dotnet-plat-ext-7.0#extension-methods
IServiceCollection can not resolve services, however. For that, we need to call
IServiceCollection.BuildServiceProvider() first, to get an IServiceProvider.
This is a different from Stride's ServiceRegistry,
which can have servces added at any time.
There was a problem hiding this comment.
When I've implemented this sort of thing in the past, I followed Microsoft's pattern whereby a List<Action<HostBuilderContext, IServiceCollection>> collection was constructed and then all assembled during a PopulateServiceCollection() method. This allowed me to call .ConfigureServices((context, services) => ...) as many times as I wanted.
| { | ||
| public ConfigurationManager Configuration { get; } | ||
| public IServiceCollection Services { get; } | ||
| public ILoggingBuilder Logging { get; } |
There was a problem hiding this comment.
ILoggingBuilder standardizes a way of configuring where to log to.
Many NuGet packages exist that work against this standard, from simple things like
files, console, Windows Event Log, to professional products like
Splunk or Sentry.
Even Slack and Telegram apparently... Maybe those are for something else.
Not gonna look into that, I doubt these will matter :D
In objects created by IServiceProvider, an ILogger<> can be received like this:
https://learn.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-7.0#create-logs
However, it will take time to move services over to IServiceProvider.
If we hook up an ILogger<> to Stride's existing logging mechanism,
people could already use all those Nuget packages mentioned above.
There was a problem hiding this comment.
Using the Microsoft ILogger<> would be nice. In many cases, it seems Stride rolled their own due to potential licensing issues. Microsoft.Extensions.Logging is MIT licensed now.
| public ConfigurationManager Configuration { get; } | ||
| public IServiceCollection Services { get; } | ||
| public ILoggingBuilder Logging { get; } | ||
| public IAppEnvironment Environment { get; } |
There was a problem hiding this comment.
ASP Core has IHostEnvironment, I created IAppEnvironment as counterpart.
I know we already have something similar, but that is only created later
after new Game() is called.
I wanted to highlight that something like this is needed as part of this GameBuilder already, for 2 reasons:
A) If reading from a file like appsettings.json for Configuration,
it needs to correctly get that file. ASP uses
IHostEnvironment.ContentRootFileProvider for this - we would need to check
if Stride's file reading stuff can already work here, without other dependencies etc.
B) Setting up the other parts of this builder usually already needs the
the info if the application is running in debug vs release mode.
"Set up profiler only if we are in debug mode" etc.
There was a problem hiding this comment.
Is there a reason we wouldn't use the term "host"? Microsoft makes a distinction between Host and WebHost in many places. They also refer to the non-web version as the "Generic Host". In this case, the Host refers to something that manages the communication with the window context and the game engine proper. Perhaps I'm taking liberties by expanding it beyond a purely network-centric definition.
Interestingly, a custom configuration provider for Stride stuff could be created. I'm not really sure what that looks like though.
There was a problem hiding this comment.
Perhaps I'm taking liberties by expanding it beyond a purely network-centric definition.
Actually, you did that exactly right. It took me some time to understand that the word "host" as they're using it in the context of "generic host" kinda has nothing to do with networking, and is more something along the lines of "the framework that 'hosts' your application on the computer".
So... "there are no solutions, only tradeoffs". If you end up going with the standard "host" naming - and taking into account that many people get into programming through game development - it would probably be a good idea to add documentation explaining this. Might save quite a few helpdesk tickets discord questions.
| using (var game = new Game()) | ||
| { | ||
| game.Run(); | ||
| } |
There was a problem hiding this comment.
I would even go a step further and say that Game IS the host. Rather than inroducing another layer in the form of this StrideApplication, you'd simply let Game be worked out in Program.cs. This would allow something like:
Game.CreateBuilder().Build().Run();and more advanced things like this:
Game.CreateBuilder()
.ConfigureServices(services => {
...
})
.Build()
.Run();This is also the way Microsoft does it here (although their Host class is much more anemic than Game): https://github.com/dotnet/runtime/blob/57ea59f57eaa66523567bbef93275fbeaceccad1/src/libraries/Microsoft.Extensions.Hosting/src/Host.cs#L75
Currently, the Game class sets up the service registry during construction. We would take the construction of the ServiceProvider into the builder which would then be passed to the Game constructor. Event handling and everything else would remain the same mostly. We just need to figure out that mutability problem...
| } | ||
|
|
||
| // Register dependencies | ||
| gameBuilder.Services.AddSingleton<Dependency>(); |
There was a problem hiding this comment.
Singleton operates as expected, same with Transient, but Scoped needs special consideration. I believe that scoping to the Scene makes sense, but that would be quite a change to things. Use of serviceProvider.CreateScope() would be needed, but where would it go, and what does it mean for EntityProcessors and ScriptComponents?
I think the larger conversation around the mutability of registered services needs resolution. What are the primary use cases for mutating the service registry after initialization? Was it only necessary because there wasn't a strong DI framework? I suspect we can afford to be more opinionated about the service registry once a proper host builder is in place.
How would EntityProcessor and ScriptComponent use the new service provider?
There was a problem hiding this comment.
I read on a blog post once that FOSS has 2 disadvantages compared to closed source software (also a bunch of advantages for sure, but the point is:):
- Work that is less "attractive" is less likely to get done
- People are much less likely to sit down in a corner of the office and draw funny shapes on a whiteboard.
IMHO using Scoped really needs the latter to add value. For example, when I discussed this with Manio, he was a proponent of a scope being a frame.
To give you my considerations from my game where I'm using this, for what it's worth:
It felt too much like "If you have a hammer, everything looks like a nail". It was certainly built with Sdk.Web in mind, and, well, think of the bread and butter for ASP, your usual B2B application: It's not out of the ordinary to have 1 million LOC, but a lot of it is edge-case stuff, like reports that are only generated once a month anyway. And a single web request only ever needs a very small part of that. Not having everything in memory constantly directly saves the company money by allowing cheaper server hosting with less RAM. Scoped just makes so much sense for such a web application.
But for a game engine, you're working with a whole different situation. Much more RAM available on average, and if it becomes a bottleneck, it's probably because of the assets, and not an overwhelming amount of code.
Honestly, I haven't encountered a hard reason for Transient in my game yet, either. My transient services are always referenced by services that are singletons in the end, so they effectively all have singleton lifetime - with the difference that multiple instances are created if it's used in multiple places. The only practical advantage I currently have is documentation a la "this class does not have state that I need to keep alive" (but that also kinda just because I decided to use it that way, for now).
Ugh, I've already written way more than I assumed this would be :D Anyway, my advice would be to just not even touch Scoped at first. If you want to do something with it, you can still add that later, the infrastructure is there - but it will become a whole own discussion.
And yeah, that kinda also goes for the EntityProcessor/ScriptComponent part, where I agree with the general sentiment that that's more of a future step. Once it is, indeed, in place - but also, sufficiently used throughout the codebase - some things will probably just "melt away" and produce a clearer picture how these things can work together.
I have a feeling already that the mechanism that's essentially activating and deactivating EntityProcessors should not be the same as the IoC container; that those are 2 distinct responsibilities... but that also strikes me as a question where you need some funny shapes on a whiteboard first.
| gameBuilder.Services.AddSingleton<Dependency>(); | ||
|
|
||
| // It is customary to use extension methods to group functionality. | ||
| gameBuilder.AddMyComponent(); |
There was a problem hiding this comment.
The idiomatic way would be to provide a series of built-in "configurators" as IHostBuilder extensions. These would facilitate the advanced startup configuration you're suggesting:
Game.CreateBuilder()
.ConfigureComponents((config, context, ...) => ...)
.ConfigureScenes(() => ...)
.Build()
.Run();Having custom extensions is fine too. My argument is that some of these things should be provided by the engine for convenience.
| <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="6.0.0" /> | ||
| <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="6.0.1" /> | ||
| <PackageReference Include="Microsoft.Extensions.Logging" Version="6.0.0" /> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="6.0.0" /> |
There was a problem hiding this comment.
Clerical task, these should now be .NET 8.
| } | ||
|
|
||
| // Again, these setup steps can be summarized into extension methods | ||
| app.UseMyComponent(); |
There was a problem hiding this comment.
This isn't a comment on your code per se, but this got me thinking about aspnet middleware and I wonder how a similar app pipeline pattern might be leveraged in the context of a game engine.
There was a problem hiding this comment.
I thought and experimented with that, too - and my result was, like another comment here "If you have a hammer...".
For a game engine, this is a solution in search of a problem; otherwise something probably immediately would have come to mind for this.
|
|
||
| public interface IGameBuilder | ||
| { | ||
| public ConfigurationManager Configuration { get; } |
There was a problem hiding this comment.
Similar to my comment below, we'd probably want them (and the engine) constructing configuration through a series of configuration actions created as a collection via the .ConfigureHostConfiguration() call or similar. This could store as a List<Action<ConfigurationBuilder>> or List<Action<HostBuilderContext, ConfigurationBuilder>>.
While building the Game instance, an IConfiguration would be built using these actions instead of relying on ConfigurationManager at runtime (which may make File I/O calls). The code below demonstrates how that could be built:
var configBuilder = new ConfigurationBuilder(); // Other extensions applied.
foreach (Action<HostBuilderContext, IConfigurationBuilder> buildAction in _configureAppConfigActions)
{
buildAction(_hostBuilderContext, configBuilder);
}
_appConfiguration = configBuilder.Build();I haven't personally used ConfigurationManager much, so I may be off base.
| using (var game = new MyGameSetup().SetUpGame()) | ||
| { | ||
| game.Run(); | ||
| } |
There was a problem hiding this comment.
Related to my above comment, this would be the point where we use:
| using (var game = new MyGameSetup().SetUpGame()) | |
| { | |
| game.Run(); | |
| } | |
| Game.CreateBuilder().Build().Run(); |
It would then be idiomatic with other .NET setups. This could also simplify the setup of cross-platform projects. I wonder how initialization of certain services, scenes, or components could be further augmented within this.
| if (gameBuilder.Environment.IsDevelopment()) | ||
| { | ||
| // set up logging | ||
| // The game imports Microsoft.Extensions.Logging.Console, | ||
| // But the engine only needs Microsoft.Extensions.Logging. | ||
| // (This example file is added to the engine so now it also needs console) | ||
| gameBuilder.Logging.AddConsole(); | ||
| } |
There was a problem hiding this comment.
Maybe I'm too puritanical. I'm not a fan of reaching into properties on a Builder. Instead, I generally opt for a closed approach relying on configurator actions. Microsoft seems to have a similar mentality on this. Having overrides that accept the HostBuilderContext would give access to Environment within the scope of the configurator action. For example:
Game.CreateBuilder().ConfigureLogging((context, builder) => {
if(context.Environment.IsDevelopment()) {
builder.AddConsole();
}
});On a side note: we make heavy use of preprocessor directives. I wonder how we could ween ourselves off this reliance by providing a more robust environment class.
| private sealed class LoggingBuilder : ILoggingBuilder | ||
| { | ||
| public LoggingBuilder(IServiceCollection services) | ||
| { | ||
| Services = services; | ||
| } | ||
|
|
||
|
|
||
| public IServiceCollection Services { get; } | ||
| } |
There was a problem hiding this comment.
Beating a dead horse, configurator actions and supplying the raw ILoggingBuilder would be better. I also don't understand why the IServiceCollection would be set here since the ServiceProvider is the thing that matters and we likely want to control the collection instance within the builder.
| private static string GetEnvironment() | ||
| { | ||
| #if DEBUG | ||
| return Environments.Development; | ||
| #elif STAGING | ||
| return Environments.Staging; | ||
| #endif | ||
| return Environments.Production; | ||
| } |
There was a problem hiding this comment.
Perhaps more control could be given back to the developers. The ASPNETCORE_ENVIRONMENT environment variable is used for those kinds of application. A custom environment variable of our own to specify the game environment could be nice. It would also give devs more flexibility for custom environment types they may have. I do believe that expecting Development or Production as reserved values would be a good idea since we would want to use them for extension methods like .IsDevelopment().
There was a problem hiding this comment.
Yeah looking back at this now, not happy with this anymore, especially since I learned that the whole idea of using Environment is to specifically not use the msbuild $(Configuration).
|
I'd like to know what's going on with that proposal |
|
I guess we might need more feedback from the community? I personally like the familiarity with other .NET projects. |
|
Hey guys I just saw that there were these comments on this PR, but uh, it's like half a year later now. I've decided to go with openGL directly instead of Stride for my 2D game some time ago already, so just as an update, I'm unable to put much more energy into this. However, on the positive side - at this point, I'm not really more familiar with these changes than another programmer familiar with the .NET generic host, so anybody can take ownership of this and not much of value is lost. |
PR Details
This is a first step for introducing an IOC container.
This would be a breaking change, so this PR is focused on only setting up the
"infrastructure" (which would be the braking change) without actually using it yet.
Description
The most important part is usage, in MyGameApp.cs
More at the issue
Related Issue
#1445
Motivation and Context
Types of changes
Checklist