feat: implementation of gestalt-DI #622
Conversation
…lApplication` with `Provider`
SolApplication is now initialised with an additional ServiceRegistry parameter for facade-provided implementation classes. This is used for providing a FacadeModuleConfig instance to the ModuleManager.
Also fixed the rapid output of "Bean no found".
… feat/experimental-DI-gestalt
[feat/experimental-DI-gestalt] Bug fixes and Android fixes
… feat/experimental-DI-gestalt-merge
BenjaminAmos
left a comment
BenjaminAmos
left a comment
There was a problem hiding this comment.
I think that this is a great step towards modularising the codebase further and removing dependencies on the monolithic SolApplication and SolGame classes. The changes made are fairly clear, although the whole gestalt-di system is going to need a lot more explicit documentation at some point.
Since this is a major change, like all other major changes, it broke the Android build. I did have some patches that fixed it again for an earlier iteration of this code but I've overhauled the Android asset loading since, so it may take some time before I can fix it again.
There are some API access exceptions thrown when trying to use code modules with these changes. Code modules are a very under-used feature of Destination Sol right now, so it is understandable that they were overlooked. In future, maybe try testing with my codeModuleTest module in the workspace, which you can obtain using groovyw module get codeTestModule -remote BenjaminAmos? I haven't even tried warp with this yet, which is a more complex code module, although you could try that one too.
With these changes, the golden asteroid is gone! Whilst it was only ever a temporary thing to test the ECS systems, the code to spawn it can still be found in SolApplication#draw. This implies that ECS code no longer works with these changes. When you start playing, a golden asteroid should (at least for now) appear in your spawn area, where you can shoot it to split it into many tiny pieces of debris.
engine/src/main/java/org/destinationsol/entitysystem/ComponentSystem.java
Show resolved
Hide resolved
engine/src/main/java/org/destinationsol/entitysystem/ComponentSystemManager.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/org/destinationsol/ui/nui/screens/MainGameScreen.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/org/destinationsol/ui/TutorialManager.java
Outdated
Show resolved
Hide resolved
pollend
changed the title
[feat/experimental-DI-gestalt-merge] Fix running the game on Android
|
I guess we need these changes for the build to work: https://github.com/MovingBlocks/DestSolAndroid |
…DI-gestalt-merge # Conflicts: # build.gradle # engine/build.gradle # engine/src/main/java/org/destinationsol/SolApplication.java
* Create BeanClassFactory for injecting to Asset related classes * Provide @Inject for every Asset related class * using scanner for scan `assets` and `ui` packages (im too lazy to explicitly write all them in service)
|
Needs MovingBlocks/TeraNUI#70 to work.
|
fix: Fix DI Loading NUI as transient service instead singleton
BenjaminAmos
left a comment
BenjaminAmos
left a comment
There was a problem hiding this comment.
It's great to have this back in a mergable state again! Some parts are still lacking explainatary javadoc but it's not worth holding this up any longer over that.
I've left a few very minor comments. Other than that, these changes are an overall improvement. Let's hope the gains from this were well worth the vast effort involved to get there.
| // TODO seems this is should be GameService before game | ||
| this.with(Console.class).lifetime(Lifetime.Singleton).use(ConsoleImpl.class); |
There was a problem hiding this comment.
This is a symptom of a far larger underlying issue with injecting NUI screens at the moment. You can't inject NUI screens from the game context right now, only from the application context.
I'm not going to block this PR over that issue though, although it's one of the more urgent ones to be resolved post-merge.
| public class EventReceiverServiceRegistry extends ServiceRegistry { | ||
| public EventReceiverServiceRegistry(ModuleEnvironment environment) { | ||
| for (Class<? extends EventReceiver> receiver : environment.getSubtypesOf(EventReceiver.class)) { | ||
| this.with(receiver).lifetime(Lifetime.Singleton); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I wrote this class. It's a compromise to get ECS working again quickly. Ideally we'd have ECS classes registered using gestalt's new service mechanisms but that's a problem for another time.
(The game's ECS classes aren't neatly put under a common package, which makes them difficult to isolate as game content rather than application/engine content)
|
|
||
| if (solGame != null) { | ||
| context.get(DrawableManager.class).draw(solGame, context); | ||
| solGame.getDrawableManager().draw(solGame, new ContextWrapper(gameContext)); |
There was a problem hiding this comment.
Creating a new ContextWrapper instance every frame seems quite wasteful. Could you cache it somewhere?
engine/src/main/java/org/destinationsol/assets/fonts/ScaledFontProducer.java
Show resolved
Hide resolved
engine/src/main/java/org/destinationsol/entitysystem/ComponentSystem.java
Show resolved
Hide resolved
| @Inject | ||
| public void setFocusManager(FocusManager focusManager) { |
There was a problem hiding this comment.
Is this an undocumented use of @Inject? Can you just annotate setters and have them automatically called at inject-time?
BenjaminAmos
left a comment
BenjaminAmos
left a comment
There was a problem hiding this comment.
The game runs successfully on both desktop and android and the build passes.
Android compilation fixes for MovingBlocks/DestinationSol#622
here is some experimental work with: https://github.com/MovingBlocks/gestalt/pull/87\
This is an implementation of gestalt DI. At the moment there is a ApplicationContext --> GameContext. Application context will contains all top level dependencies such as the audio system and the game context is the actual gameplay systems. This could be managed and split into different layers depending on different states of the application. each module has its own context so
This is how the current import scheme works with Gestalt modules so each module is only dependent on a child module and so forth. there is no way that moduel2 has a cyclic dependency on Module1
This is a straightforward replacement of the context system that was pulled in from Terasology. Each context defines the services that are needed and at request the object is built up from the dependencies within that context and the parent context if it fails to get resolved in the current context. anything that used to be dependent on pulling from the context will declare the dependencies that are needed to build that object and the context will resolve building that object.
Unit tests are a matter of providing a serviceRegistry with the needed dependencies and the injection system will resolve those as need be. it should be easy enough to construct a mock object within the context and the system should handle the rest. reduces the burden of working backwards from the source to construct a test environment.
This handles a lot of the clear problems with the old scheme.
A lot of the code that is used to construct the game is gone and and has been handed over the DI system.
This also does away with reflections. this is also managed by gestalt-DI. I think @DarkWeird would have more information about that portion of the implementation. basically lets us do some scanning for a given type without paying the cost of walking the class path. accomplishes a similar goal to reflections bug handled through the annotation processor.
need changes from this PR: MovingBlocks/TeraNUI#65
this PR replaces this PR: #586