Conversation
Removes unnecessary and too early registration step and fixes a whitelist issue
Physics is wired differently, this appears to have no functional effect
Contributor
Author
|
The tests presently all pass and CI considers it acceptable, so this could be theoretically be merged by this point. In reality, I need to clean it up just a bit more and test it more thoroughly to check for less obvious regressions. It is pretty much there though now. |
BenjaminAmos
changed the title
BenjaminAmos
commented
Mar 25, 2026
| } | ||
|
|
||
| @Inject | ||
| public BlockFamilyLibrary(ModuleManager moduleManager, Context context) { |
Contributor
Author
There was a problem hiding this comment.
We should not be injecting Context directly unless absolutely necessary. It appears unncessary in this case.
| TypeWidgetLibrary moduleLibrary = | ||
| new TypeWidgetLibraryImpl(typeWidgetFactoryRegistry, declaringModule, this.context); | ||
| context.put(TypeWidgetLibrary.class, moduleLibrary); | ||
| new TypeWidgetLibraryImpl(typeWidgetFactoryRegistry, declaringModule, timedContextForModulesWidgets); |
Contributor
Author
There was a problem hiding this comment.
timedContextForModulesWidgets? Does that name even make sense here?
| private final Context context; | ||
|
|
||
| @Inject | ||
| public ConsoleImpl(Context context) { |
Contributor
Author
There was a problem hiding this comment.
This class does not require a full context. Inject NetworkSystem and PermissionManager instead.
| CoreRegistry.put(GameEngine.class, host); | ||
| Context coreContextOverride = new ContextImpl(CoreRegistry.get(Context.class)); | ||
| coreContextOverride.put(GameEngine.class, host); | ||
| CoreRegistry.setContext(coreContextOverride); |
Contributor
Author
There was a problem hiding this comment.
In general, adding new CoreRegistry uses is ill-advised, however, this is in the test environment and so is less important. Presumably it fixed some failing unit tests.
| boolean permissiveSecurityEnabled = Boolean.parseBoolean(System.getProperty(SystemConfig.PERMISSIVE_SECURITY_ENABLED_PROPERTY)); | ||
| if (permissiveSecurityEnabled) { | ||
| newEnvironment = new ModuleEnvironment(new DefaultBeanContext(), finalModules, wrappingPermissionProviderFactory); | ||
| newEnvironment = new ModuleEnvironment(((ContextImpl) context).getBeanContext(), finalModules, wrappingPermissionProviderFactory); |
Contributor
Author
There was a problem hiding this comment.
Should this implementation requirement be documented?
Comment on lines
+124
to
+125
| () -> CoreRegistry.get(ChunkProvider.class), () -> CoreRegistry.get(NetworkSystem.class), CoreRegistry.get(Time.class), | ||
| CoreRegistry.get(Game.class)); |
Contributor
Author
There was a problem hiding this comment.
This constructor should be deprecated. Implicit dependencies on CoreRegistry often lead to unexpected behaviour.
| * | ||
| * @param context a context object, to obtain instances of classes such as the rendering config. | ||
| */ | ||
| @Inject |
Contributor
Author
There was a problem hiding this comment.
The context parameter in this constructor should be replaced with explicit parameters for each dependency needed from the context.
| try { | ||
| worldGenerator = WorldGeneratorManager.createGenerator(worldInfo.getWorldGenerator(), context); | ||
| // setting the world seed will create the world builder | ||
| worldGenerator.setWorldSeed(worldInfo.getSeed()); |
Contributor
Author
There was a problem hiding this comment.
Where is the seed being set now?
Contributor
Author
There was a problem hiding this comment.
This file contains some of what I would consider the most impactful but also problematic changes. The fact that multiplayer does not work correctly is likely due to certain changes to this class that were not entirely backwards compatible with the previous implementation.
Member
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis pull request refactors Terasology's dependency injection model from Context-based to Gestalt ServiceRegistry-based approach. It introduces service registration patterns, dependency-injected constructors, and new immutable context support across engine subsystems, load processes, entity systems, and testing infrastructure, alongside documentation of engine startup and state-machine flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR migrates large parts of the Terasology engine from CoreRegistry/manual wiring to Gestalt DI, reworking engine bring-up into registry-backed phases and refactoring many subsystems, world initialisation, and tests to match.
Changes:
- Introduces
ServiceRegistry-based subsystem registration and new context variants (ContextImplbacked by Gestalt beans, plus an immutable context). - Refactors world/game loading steps to register services via DI (blocks, world providers, rendering, physics, etc.) instead of
Context.put/CoreRegistry. - Updates broad swathes of engine + test harnesses to construct and configure the entity system, configs, and module environments under the new model.
Key review findings (blocking):
PhysicsEngineManager.registerPhysicsEngine(...)does not actually register a constructible physics engine (missing.use(...), and no binding forPhysics/PhysicsEngine). This will make physics injection/lookup fail at runtime.UniverseSetupScreencreates a brand-newUniverseWrapperininitialise()and does not apply the wrapper passed tosetEnvironment(...). This breaks seed/settings handoff fromAdvancedGameSetupScreenand can desync preview vs. “Play” state.AdvancedGameSetupScreendereferencesuniverseWrapperwithout guaranteeing it’s set (NPE risk if the screen is opened through any path that doesn’t callsetEnvironmentfirst).
Reviewed changes
Copilot reviewed 137 out of 137 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java | Avoids invoking null getters for private fields during persistence serialization. |
| subsystems/DiscordRPC/src/main/java/org/terasology/subsystem/discordrpc/DiscordRPCSubSystem.java | Migrates Discord RPC subsystem config access to DI. |
| engine/src/main/java/org/terasology/engine/world/sun/DefaultCelestialSystem.java | Adds DI constructor wiring for celestial system. |
| engine/src/main/java/org/terasology/engine/world/sun/BasicCelestialModel.java | Adds injectable no-arg constructor. |
| engine/src/main/java/org/terasology/engine/world/internal/WorldProviderWrapper.java | Shifts delegation from a stored core field to decorator base. |
| engine/src/main/java/org/terasology/engine/world/internal/WorldProviderCoreImpl.java | Removes Context dependency; injects EntityManager/BlockManager for DI-friendly construction. |
| engine/src/main/java/org/terasology/engine/world/internal/EntityAwareWorldProvider.java | Adds DI constructor and removes reliance on context wiring. |
| engine/src/main/java/org/terasology/engine/world/internal/AbstractWorldProviderDecorator.java | Makes base protected for downstream decorators. |
| engine/src/main/java/org/terasology/engine/world/generator/plugin/DefaultWorldGeneratorPluginLibrary.java | Adds DI-friendly constructors (ModuleManager-based). |
| engine/src/main/java/org/terasology/engine/world/generator/internal/WorldGeneratorManager.java | Refactors to depend directly on ModuleManager rather than Context. |
| engine/src/main/java/org/terasology/engine/world/chunks/remoteChunkProvider/RemoteChunkProvider.java | Marks constructor for injection. |
| engine/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/RelevanceSystem.java | Marks constructor for injection. |
| engine/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.java | Adds DI constructor and concurrent map default. |
| engine/src/main/java/org/terasology/engine/world/chunks/blockdata/ExtraBlockDataManager.java | Refactors annotation scanning to accept environment + block manager (DI-ready). |
| engine/src/main/java/org/terasology/engine/world/block/internal/BlockManagerImpl.java | Updates BlockBuilder construction to include AssetManager. |
| engine/src/main/java/org/terasology/engine/world/block/internal/BlockBuilder.java | Adds AssetManager-based constructor and deprecates atlas-only constructor. |
| engine/src/main/java/org/terasology/engine/world/block/family/BlockFamilyLibrary.java | Adds DI constructor using ModuleManager environment. |
| engine/src/main/java/org/terasology/engine/rendering/world/viewDistance/ClientViewDistanceSystem.java | Updates TranslationSystem wiring to new ctor dependencies. |
| engine/src/main/java/org/terasology/engine/rendering/world/WorldRendererImpl.java | Reworks renderer bring-up to DI/context registry model; adds explicit init(). |
| engine/src/main/java/org/terasology/engine/rendering/world/WorldRenderer.java | Adds init() lifecycle hook for renderers. |
| engine/src/main/java/org/terasology/engine/rendering/world/RenderableWorldImpl.java | Makes RenderableWorld injectable + public; replaces CoreRegistry usage. |
| engine/src/main/java/org/terasology/engine/rendering/world/RenderQueuesHelper.java | Makes constructor public for external construction/DI. |
| engine/src/main/java/org/terasology/engine/rendering/opengl/fbms/DisplayResolutionDependentFbo.java | Adds Config-based injectable constructor. |
| engine/src/main/java/org/terasology/engine/rendering/opengl/ScreenGrabber.java | Adds Config-based injectable constructor. |
| engine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/advancedGameSetupScreen/AdvancedGameSetupScreen.java | Removes CoreRegistry usage; keeps UniverseWrapper as instance field. |
| engine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/UniverseSetupScreen.java | Removes CoreRegistry usage; introduces UniverseWrapper instance field and DI changes. |
| engine/src/main/java/org/terasology/engine/rendering/nui/internal/NUIManagerInternal.java | Adjusts widget injection context use and adds performance monitoring activities. |
| engine/src/main/java/org/terasology/engine/rendering/nui/internal/LwjglCanvasRenderer.java | Refactors ctor to injected dependencies rather than Context. |
| engine/src/main/java/org/terasology/engine/rendering/dag/RenderGraph.java | Marks ctor injectable. |
| engine/src/main/java/org/terasology/engine/rendering/cameras/PerspectiveCamera.java | Adds Config-based injectable constructor. |
| engine/src/main/java/org/terasology/engine/rendering/backdrop/Skysphere.java | Adds injectable constructor and delegates Context ctor. |
| engine/src/main/java/org/terasology/engine/recording/RecordingClasses.java | New wrapper type to DI-provide record/replay class lists. |
| engine/src/main/java/org/terasology/engine/recording/RecordAndReplaySerializer.java | Marks ctor injectable. |
| engine/src/main/java/org/terasology/engine/recording/EventSystemReplayImpl.java | Adds injectable ctor that consumes RecordingClasses. |
| engine/src/main/java/org/terasology/engine/recording/EventCatcher.java | Adds injectable ctor that consumes RecordingClasses. |
| engine/src/main/java/org/terasology/engine/physics/engine/PhysicsEngineManager.java | Adds DI registration entrypoint for physics engine. |
| engine/src/main/java/org/terasology/engine/physics/bullet/BulletPhysics.java | Converts to DI ctor for BlockEntityRegistry; implements Physics. |
| engine/src/main/java/org/terasology/engine/persistence/typeHandling/TypeHandlerLibraryImpl.java | Makes library injectable and ensures default handlers are populated. |
| engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java | Adds DI ctor and replaces some CoreRegistry lookups with Providers. |
| engine/src/main/java/org/terasology/engine/network/internal/ServerConnectListManager.java | Marks ctor injectable. |
| engine/src/main/java/org/terasology/engine/network/internal/NetworkSystemImpl.java | Marks ctor injectable. |
| engine/src/main/java/org/terasology/engine/logic/console/ConsoleImpl.java | Marks ctor injectable. |
| engine/src/main/java/org/terasology/engine/identity/storageServiceClient/StorageServiceWorker.java | Refactors ctor to explicit Config + TranslationSystem dependencies. |
| engine/src/main/java/org/terasology/engine/i18n/TranslationSystemImpl.java | Refactors ctor to SystemConfig + AssetManager (DI-friendly). |
| engine/src/main/java/org/terasology/engine/i18n/I18nSubsystem.java | Moves TranslationSystem registration to ServiceRegistry preInitialise. |
| engine/src/main/java/org/terasology/engine/entitySystem/prefab/internal/PojoPrefabManager.java | Adds injectable ctor. |
| engine/src/main/java/org/terasology/engine/entitySystem/metadata/EventLibrary.java | Adds DI ctor using ModuleManager environment supplier. |
| engine/src/main/java/org/terasology/engine/entitySystem/metadata/EntitySystemLibrary.java | Adds DI ctor and keeps legacy Context-based ctor. |
| engine/src/main/java/org/terasology/engine/entitySystem/metadata/ComponentLibrary.java | Adds DI ctor using ModuleManager environment supplier. |
| engine/src/main/java/org/terasology/engine/entitySystem/event/internal/EventSystemImpl.java | Injects NetworkSystem for authority mode and fixes typo in field name. |
| engine/src/main/java/org/terasology/engine/entitySystem/entity/internal/PojoEntityManager.java | Adds injectable ctor and makes it implement EntityManager directly. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglTimer.java | Migrates time subsystem to ServiceRegistry-based preInitialise. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglRenderingSubsystemFactory.java | Registers renderer graph/services via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglInput.java | Registers InputSystem via ServiceRegistry; uses existing instance in post init. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglGraphicsManager.java | Registers RenderingSubsystemFactory via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglGraphics.java | Migrates graphics subsystem to ServiceRegistry; injects root Config. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglDisplayDevice.java | Refactors ctor to take RenderingConfig directly. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglAudio.java | Migrates audio subsystem to ServiceRegistry; injects AudioConfig. |
| engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/BaseLwjglSubsystem.java | Updates preInitialise signature for ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/headless/renderer/HeadlessWorldRenderer.java | Adds injectable ctor and implements init(). |
| engine/src/main/java/org/terasology/engine/core/subsystem/headless/renderer/HeadlessRenderingSubsystemFactory.java | Registers headless renderer via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/headless/mode/StateHeadlessSetup.java | Refactors state bring-up to serviceRegistry + immutable context approach. |
| engine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessTimer.java | Migrates headless timer to ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessInput.java | Adds injectable ctor. |
| engine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessGraphics.java | Migrates headless graphics wiring to ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessAudio.java | Migrates headless audio wiring to ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/config/BindsSubsystem.java | Moves binds manager provisioning to ServiceRegistry; makes deps optional. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/hibernation/HibernationSubsystem.java | Registers HibernationManager via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/WorldGenerationSubsystem.java | Registers WorldGeneratorManager via ServiceRegistry and injects ModuleManager. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/TelemetrySubSystem.java | Registers Metrics + Emitter via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/PhysicsSubsystem.java | Registers CollisionGroupManager via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/NetworkSubsystem.java | Registers NetworkSystem via ServiceRegistry; keeps postInitialise wiring. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/MonitoringSubsystem.java | Refactors monitoring init to postInitialise with injected deps. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/GameSubsystem.java | Registers Game via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/ConfigurationSubsystem.java | Refactors config creation and StorageServiceWorker setup to DI model. |
| engine/src/main/java/org/terasology/engine/core/subsystem/common/CommandSubsystem.java | Registers ParameterAdapterManager via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/RenderingSubsystemFactory.java | Changes factory API to register via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/subsystem/EngineSubsystem.java | Updates lifecycle method signatures to use ServiceRegistry where applicable. |
| engine/src/main/java/org/terasology/engine/core/module/ModuleManager.java | Allows environment load/resolve with ContextImpl bean context. |
| engine/src/main/java/org/terasology/engine/core/module/ExternalApiWhitelist.java | Expands whitelist for prefab + persistence internals. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterWorldSystems.java | New: registers world systems + calls WorldRenderer.init(). |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterSystems.java | Hooks prefab manager into PojoEntityManager after system load. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterRemoteWorldSystems.java | New: registers remote-world systems and initialises renderer. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterMods.java | Updates module environment switch handler signature to include ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterBlocks.java | Refactors block registration to ServiceRegistry and defers init to new step. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/LoadExtraBlockData.java | Registers ExtraBlockDataManager via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.java | Updates environment loading and context creation for client join flow. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseWorld.java | Refactors world wiring to ServiceRegistry, introduces WorldProviderCore workaround type. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRendering.java | Registers RenderingModuleRegistry via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRemoteWorld.java | Refactors remote world wiring to ServiceRegistry with WorldProviderCore workaround. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialisePhysics.java | Switches physics init to registry-based registration. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseEntitySystem.java | Routes entity system setup through ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseComponentSystemManager.java | Registers ComponentSystemManager via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseCommandSystem.java | Registers Console via ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseBlocks.java | New: runs BlockManagerImpl.initialise(...) using the manifest’s maps. |
| engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/ConfigureEntitySystem.java | New: calls EntitySystemSetupUtil configuration after DI creation. |
| engine/src/main/java/org/terasology/engine/core/modes/StateMainMenu.java | Reworks main menu state initialisation around ServiceRegistry + immutable context. |
| engine/src/main/java/org/terasology/engine/core/modes/AbstractState.java | Splits registration vs init phases for entity/component managers and local player creation. |
| engine/src/main/java/org/terasology/engine/core/bootstrap/EnvironmentSwitchHandler.java | Refactors game-environment switch to populate a game registry instead of mutating context directly. |
| engine/src/main/java/org/terasology/engine/core/bootstrap/ClassMetaLibraryImpl.java | Removes Context dependency; uses ModuleManager directly. |
| engine/src/main/java/org/terasology/engine/core/GameEngine.java | Adds child context creation overloads that accept ServiceRegistry and immutable child context creation. |
| engine/src/main/java/org/terasology/engine/core/ComponentSystemManager.java | Marks ctor injectable. |
| engine/src/main/java/org/terasology/engine/context/internal/ImmutableContextImpl.java | New: context variant that forbids put. |
| engine/src/main/java/org/terasology/engine/context/internal/ContextImpl.java | Re-implements Context on top of Gestalt BeanContext + keeps map fallback. |
| engine/src/main/java/org/terasology/engine/config/flexible/AutoConfigManager.java | Refactors AutoConfig loading to register into a ServiceRegistry. |
| engine/src/main/java/org/terasology/engine/config/InputConfig.java | Removes MockContext dependency when resetting to defaults. |
| engine/src/main/java/org/terasology/engine/config/Config.java | Removes Context dependency from Config ctor. |
| engine-tests/src/test/java/org/terasology/math/IntMathTest.java | Updates config construction (no MockContext). |
| engine-tests/src/test/java/org/terasology/engine/world/generator/internal/WorldGeneratorManagerTest.java | Updates WorldGeneratorManager ctor usage. |
| engine-tests/src/test/java/org/terasology/engine/rendering/nui/widgets/treeView/JsonTreeConverterTest.java | Fixes URL decoding for resource files. |
| engine-tests/src/test/java/org/terasology/engine/rendering/nui/layers/mainMenu/savedGames/GameProviderTest.java | Fixes URL decoding for resource files. |
| engine-tests/src/test/java/org/terasology/engine/rendering/nui/editor/ContextMenuUtilsTest.java | Updates TranslationSystem ctor usage and fixes URL decoding. |
| engine-tests/src/test/java/org/terasology/engine/persistence/internal/StorageManagerTest.java | Refactors test wiring to ServiceRegistry + new storage manager ctor. |
| engine-tests/src/test/java/org/terasology/engine/persistence/EntitySerializerTest.java | Refactors entity serializer test harness to ServiceRegistry and DI. |
| engine-tests/src/test/java/org/terasology/engine/persistence/ComponentSerializerTest.java | Refactors component serializer tests to ServiceRegistry and DI. |
| engine-tests/src/test/java/org/terasology/engine/network/internal/NetworkOwnershipTest.java | Refactors test wiring to ServiceRegistry and DI. |
| engine-tests/src/test/java/org/terasology/engine/integrationenvironment/CustomSubsystemTest.java | Updates custom subsystem init to postInitialise + injection. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/PrefabTest.java | Refactors prefab tests to ServiceRegistry + DI wiring. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityPoolTest.java | Refactors entity pool tests to ServiceRegistry + DI wiring. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityManagerTest.java | Refactors entity manager tests to ServiceRegistry + DI wiring. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/OwnershipHelperTest.java | Refactors ownership helper tests to ServiceRegistry + DI wiring. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/BaseEntityRefTest.java | Refactors entity ref tests to ServiceRegistry + DI wiring. |
| engine-tests/src/test/java/org/terasology/engine/core/subsystem/config/BindsSubsystemTest.java | Refactors binds subsystem tests to ServiceRegistry + DI injection. |
| engine-tests/src/test/java/org/terasology/engine/config/flexible/AutoConfigManagerTest.java | Updates to new AutoConfigManager API (environment + ServiceRegistry). |
| engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java | Refactors headless test environment setup to ServiceRegistry + DI. |
| engine-tests/src/main/java/org/terasology/unittest/stubs/StubSubsystem.java | Makes stub subsystem injectable/public ctor. |
| engine-tests/src/main/java/org/terasology/engine/testUtil/WithUnittestModule.java | Registers unittest module via ModuleManager with ContextImpl + updates default mod selection. |
| engine-tests/src/main/java/org/terasology/engine/testUtil/ModuleManagerFactory.java | Ensures module manager includes Gestalt prefab types; loads env with ContextImpl. |
| engine-tests/src/main/java/org/terasology/engine/integrationenvironment/IntegrationEnvironmentSubsystem.java | Migrates to injected deps + ServiceRegistry-based init. |
| engine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.java | Adjusts CoreRegistry usage by creating override contexts for engine/client. |
| engine-tests/src/main/java/org/terasology/engine/Environment.java | Refactors test environment reset to ServiceRegistry-based wiring and post-build context creation. |
| docs/EngineStartUp/Engine-States.md | New: documents engine state flow and load process pipeline (mermaid). |
| docs/EngineStartUp/Engine-Start-Up.md | New: documents 3-phase engine startup rationale and revised sequence (mermaid). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java (1)
121-126:⚠️ Potential issue | 🔴 CriticalCritical bugs in deserialize: missing null check and incorrect setter invocation.
The serialize fix added a null check for
findGetter, but the parallel code indeserializehas the same NPE risk withfindSetter. Additionally, line 123 has a more severe bug: the setter is invoked without passing the value to set.
- NPE risk:
findSetter(field)can return null (same asfindGetter), but no null check exists.- Missing value argument:
invoke(result)invokes the setter with no arguments. Setters require the value: should beinvoke(result, fieldValue.get()).This means private fields will fail to deserialize correctly.
🐛 Proposed fix
if (Modifier.isPrivate(field.getModifiers())) { + Method setter = ReflectionUtil.findSetter(field); + if (setter != null) { + try { + setter.invoke(result, fieldValue.get()); + } catch (InvocationTargetException e) { + logger.error("Failed to invoke setter for field {}", field); + } + } else { + logger.error("Field {} is inaccessible - private and has no setter.", field); + } - try { - ReflectionUtil.findSetter(field).invoke(result); - } catch (InvocationTargetException e) { - logger.error("Failed to invoke setter for field {}", field); - } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java` around lines 121 - 126, The deserialize code calls ReflectionUtil.findSetter(field) without checking for null and invokes the returned Method as invoke(result) with no argument; update the block handling private fields in ObjectFieldMapTypeHandler.deserialize to first assign Method setter = ReflectionUtil.findSetter(field) and if setter is null log a warning/error and skip, otherwise call setter.invoke(result, fieldValue.get()) (preserving existing try/catch for InvocationTargetException/IllegalAccessException) so the setter receives the value and avoids NPEs.engine/src/main/java/org/terasology/engine/persistence/typeHandling/TypeHandlerLibraryImpl.java (1)
96-119:⚠️ Potential issue | 🟠 MajorRemove duplicate handler population in
forModuleEnvironment().The
@Injectconstructor now callspopulateWithDefaultHandlers(this)at line 101. However,forModuleEnvironment()(lines 113-119) creates an instance via this same constructor and then callspopulateWithDefaultHandlers(library)again at line 116, causing handlers to be registered twice. TheaddTypeHandlerFactory()method simply appends to a list without deduplication, so each call adds duplicate factory instances.Proposed fix: Remove duplicate call in forModuleEnvironment()
public static TypeHandlerLibrary forModuleEnvironment(ModuleManager moduleManager, TypeRegistry typeRegistry) { TypeHandlerLibrary library = new TypeHandlerLibraryImpl(moduleManager, typeRegistry); - populateWithDefaultHandlers(library); - return library; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/persistence/typeHandling/TypeHandlerLibraryImpl.java` around lines 96 - 119, The forModuleEnvironment factory currently instantiates TypeHandlerLibraryImpl (which already calls populateWithDefaultHandlers in its `@Inject` constructor) and then calls populateWithDefaultHandlers(library) again, duplicating registrations; remove the second call to populateWithDefaultHandlers from the forModuleEnvironment method so that TypeHandlerLibraryImpl’s constructor is solely responsible for populating defaults (references: TypeHandlerLibraryImpl, forModuleEnvironment, populateWithDefaultHandlers).engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.java (1)
65-70:⚠️ Potential issue | 🔴 CriticalDon't treat a dead switch thread as a successful environment change.
This handoff has no real failure channel. A missing
BeanContextor any exception inhandleSwitchToGameEnvironment(...)/new ContextImpl(...)will either fail opaquely or look like success once the thread exits. Additionally,CoreRegistry.setContext(...)is published from the worker thread instead of the main load path, and the unsafe.stream().findFirst().get()has no null-safety fallback. Please capture the result/exception, surface a clean join failure, and only swap the global context after the task completes successfully.Lines 123–130 have the same issue: the
BeanContextextraction and thread launch both lack error handling.🧵 Suggested shape for a safer handoff
- private Thread applyModuleThread; + private FutureTask<Context> applyModuleTask; @@ - if (applyModuleThread != null) { - if (!applyModuleThread.isAlive()) { + if (applyModuleTask != null) { + if (applyModuleTask.isDone()) { + try { + CoreRegistry.setContext(applyModuleTask.get()); + } catch (ExecutionException e) { + StateMainMenu mainMenu = new StateMainMenu("Failed to apply game environment: " + e.getCause().getMessage()); + context.get(GameEngine.class).changeState(mainMenu); + networkSystem.shutdown(); + return false; + } if (oldEnvironment != null) { oldEnvironment.close(); } return true; } return false; } @@ - ContextImpl modulesContext = new ContextImpl(context, - moduleManager.getEnvironment().getBeans(BeanContext.class).stream().findFirst().get()); + BeanContext beanContext = moduleManager.getEnvironment().getBeans(BeanContext.class).stream() + .findFirst() + .orElseThrow(() -> new IllegalStateException("Loaded module environment did not expose a BeanContext")); + ContextImpl modulesContext = new ContextImpl(context, beanContext); ServiceRegistry gameContextRegistry = new ServiceRegistry(); - applyModuleThread = new Thread(() -> { - environmentSwitchHandler.handleSwitchToGameEnvironment(modulesContext, gameContextRegistry); - Context gameContext = new ContextImpl(modulesContext, gameContextRegistry); - CoreRegistry.setContext(gameContext); - }); - applyModuleThread.start(); + applyModuleTask = new FutureTask<>(() -> { + environmentSwitchHandler.handleSwitchToGameEnvironment(modulesContext, gameContextRegistry); + return new ContextImpl(modulesContext, gameContextRegistry); + }); + new Thread(applyModuleTask, "apply-module-environment").start();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.java` around lines 65 - 70, The current handoff treats a terminated applyModuleThread as success and publishes CoreRegistry.setContext(...) from the worker with unsafe BeanContext extraction (stream().findFirst().get()), so change JoinServer to capture the worker task’s result/exception (wrap handleSwitchToGameEnvironment(...) and new ContextImpl(...) in a Callable/Future), perform null-safe BeanContext extraction (avoid .get() without a fallback), only call CoreRegistry.setContext(...) from the main load path after the Future completes successfully, and surface a clean join failure if the task throws or returns a bad result; also apply the same safe error capture and null-checks to the code around the BeanContext extraction and thread launch in the region referenced (lines ~123–130) so the environment swap only occurs on explicit success.engine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.java (1)
272-274:⚠️ Potential issue | 🟡 MinorInconsistent context handling: direct
CoreRegistry.putvs. override context pattern.Line 273 uses
CoreRegistry.put(GameEngine.class, host)directly, while the setup at lines 111-113 and connectToHost at lines 301-312 use the override context pattern. This inconsistency could cause theGameEngineto be registered in the wrong context duringStateLoading.♻️ Suggested fix to use consistent pattern
} else if (newState instanceof StateLoading) { - CoreRegistry.put(GameEngine.class, host); + Context ctx = CoreRegistry.get(Context.class); + if (ctx != null) { + ctx.put(GameEngine.class, host); + } }Alternatively, this registration may no longer be needed if
GameEngineis already available via the override context set insetup().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.java` around lines 272 - 274, The StateLoading branch directly calls CoreRegistry.put(GameEngine.class, host) which is inconsistent with the override context pattern used in setup() and connectToHost(); change this to register GameEngine via the same override context mechanism (use the override context helper/registration used around connectToHost and setup) or remove the registration entirely if GameEngine is already provided by the override context; update the StateLoading handling (referencing StateLoading and CoreRegistry.put(GameEngine.class, host)) to use the override context API used elsewhere so registration occurs in the correct context.engine/src/main/java/org/terasology/engine/core/subsystem/config/BindsSubsystem.java (1)
421-430:⚠️ Potential issue | 🟡 MinorPotential NPE:
mouseWheelUpBindandmouseWheelDownBindmay be null.These fields are set to
nullinclearBinds()(lines 228-229) and may not be re-assigned if no wheel bindings are registered. Accessing.getId()on them will throw NPE.🛡️ Proposed fix: Add null checks
- if (mouseWheelUpBind.getId().equals(bindId)) { + if (mouseWheelUpBind != null && mouseWheelUpBind.getId().equals(bindId)) { inputs.add(MouseInput.WHEEL_UP); } - if (mouseWheelDownBind.getId().equals(bindId)) { + if (mouseWheelDownBind != null && mouseWheelDownBind.getId().equals(bindId)) { inputs.add(MouseInput.WHEEL_DOWN); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/subsystem/config/BindsSubsystem.java` around lines 421 - 430, The code accesses mouseWheelUpBind.getId() and mouseWheelDownBind.getId() which can be null after clearBinds(); update the logic in BindsSubsystem (the method containing those ifs) to guard against nulls by checking mouseWheelUpBind != null and mouseWheelDownBind != null before calling getId() (or use a null-safe equals pattern) so no NPE is thrown when wheel binds were not re-registered; reference the fields mouseWheelUpBind, mouseWheelDownBind and the clearBinds() behavior when making the change.engine-tests/src/test/java/org/terasology/engine/entitySystem/BaseEntityRefTest.java (1)
49-49:⚠️ Potential issue | 🟠 MajorReset each test from a stable base context and update
CoreRegistry.
contextis wrapped again in every@BeforeEach, so the parent chain keeps growing across the suite.CoreRegistryalso remains pointed at the original base context, which means anyCoreRegistrylookup during a test sees stale services instead of the fresh per-test ones.🔧 Suggested fix
public class BaseEntityRefTest { - private static Context context; + private static Context baseContext; + private static Context context; @@ - context = new ContextImpl(serviceRegistry); - CoreRegistry.setContext(context); + baseContext = new ContextImpl(serviceRegistry); + context = baseContext; + CoreRegistry.setContext(context); @@ - context = new ContextImpl(context, serviceRegistry); + context = new ContextImpl(baseContext, serviceRegistry); + CoreRegistry.setContext(context); EntitySystemSetupUtil.configureEntityManagementRelatedClasses(context.get(TypeHandlerLibrary.class),Also applies to: 55-65, 73-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/entitySystem/BaseEntityRefTest.java` at line 49, The test suite is repeatedly wrapping the same static Context which grows the parent chain and leaves CoreRegistry pointed at the stale original; change the test setup so each test gets a fresh per-test context (do not reuse the static context variable) by creating a new child or new Context in the `@BeforeEach` for BaseEntityRefTest and assign that instance to the test-level context variable (remove static) and then update CoreRegistry to point to this fresh context (e.g., CoreRegistry.setContext(...) or the project equivalent) so lookups in each test use the fresh services; ensure the stable base context is kept immutable and used only as the parent when creating per-test children to avoid accumulation.engine-tests/src/test/java/org/terasology/engine/persistence/EntitySerializerTest.java (1)
59-60:⚠️ Potential issue | 🟠 MajorKeep a dedicated root context instead of chaining test contexts.
setup()currently uses the previous test'scontextas the parent for the next one, so missing registrations can be satisfied by fallback from earlier tests.CoreRegistryalso stays pinned to the old root. Rebuild from a stable root context each time.Suggested change
public class EntitySerializerTest { - private static Context context; + private static Context rootContext; private static ModuleManager moduleManager; + private Context context; private ComponentLibrary componentLibrary; @@ - context = new ContextImpl(serviceRegistry); - CoreRegistry.setContext(context); + rootContext = new ContextImpl(serviceRegistry); + CoreRegistry.setContext(rootContext); @@ - context = new ContextImpl(context, serviceRegistry); + context = new ContextImpl(rootContext, serviceRegistry); + CoreRegistry.setContext(context);Also applies to: 69-80, 83-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/persistence/EntitySerializerTest.java` around lines 59 - 60, The tests currently chain contexts causing registrations to leak between runs; update setup() to create a fresh root Context (not reuse the previous static context) and build a new test Context from that root each time, register ModuleManager into the new Context (moduleManager) and replace the CoreRegistry root binding so CoreRegistry points at the new Context; ensure any tearDown/cleanup clears CoreRegistry or resets the static context so subsequent tests start from a clean root rather than falling back to prior registrations.engine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityPoolTest.java (1)
41-41:⚠️ Potential issue | 🟠 MajorReset the per-test context from a stable root.
setup()currently wraps the previous test'scontext, so every test can fall back to services/state installed by earlier tests.CoreRegistryalso stays pinned to the original root. Keep a separate root context and create a fresh child from that in each@BeforeEach.Suggested change
public class PojoEntityPoolTest { - private static Context context; + private static Context rootContext; + private Context context; private PojoEntityPool pool; private PojoEntityManager entityManager; @@ - context = new ContextImpl(serviceRegistry); - CoreRegistry.setContext(context); + rootContext = new ContextImpl(serviceRegistry); + CoreRegistry.setContext(rootContext); @@ - EntitySystemSetupUtil.addEntityManagementRelatedClasses(context, serviceRegistry); - context = new ContextImpl(context, serviceRegistry); + EntitySystemSetupUtil.addEntityManagementRelatedClasses(rootContext, serviceRegistry); + context = new ContextImpl(rootContext, serviceRegistry); + CoreRegistry.setContext(context);Also applies to: 47-59, 62-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityPoolTest.java` at line 41, The tests currently reuse a wrapped Context across runs so state bleeds between tests; introduce a stable root Context (e.g., a static rootContext) that is initialized once and then in setup() create a fresh child Context assigned to the test-scoped variable `context`, and make sure to reset CoreRegistry to point at that fresh child rather than the original root; update any test teardown to clear/reset `context` and restore CoreRegistry to the stable root if needed so each `@BeforeEach` gets a clean, isolated Context.engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java (1)
269-275:⚠️ Potential issue | 🟠 MajorExplicit saves can still crash on unresolved providers.
update()guards autosaves viaisRunModeAllowSaving(), butwaitForCompletionOfPreviousSaveAndStartSaving()goes straight throughstartSaving()to this method. The late-boundchunkProviderandnetworkSystemare dereferenced unconditionally here, so a save during a transition becomes an NPE instead of a clear failure.Minimal hardening
private SaveTransaction createSaveTransaction() { + ChunkProvider chunkProviderInstance = chunkProvider != null ? chunkProvider.get() : null; + NetworkSystem networkSystemInstance = networkSystem != null ? networkSystem.get() : null; + if (chunkProviderInstance == null || networkSystemInstance == null) { + throw new IllegalStateException("Cannot create a save transaction without ChunkProvider and NetworkSystem"); + } + SaveTransactionBuilder saveTransactionBuilder = new SaveTransactionBuilder(privateEntityManager, entitySetDeltaRecorder, isStoreChunksInZips(), getStoragePathProvider(), worldDirectoryWriteLock, recordAndReplaySerializer, recordAndReplayUtils, recordAndReplayCurrentStatus); - addChunksToSaveTransaction(saveTransactionBuilder, chunkProvider.get()); - addPlayersToSaveTransaction(saveTransactionBuilder, networkSystem.get()); + addChunksToSaveTransaction(saveTransactionBuilder, chunkProviderInstance); + addPlayersToSaveTransaction(saveTransactionBuilder, networkSystemInstance); addGlobalStoreBuilderToSaveTransaction(saveTransactionBuilder); addGameManifestToSaveTransaction(saveTransactionBuilder);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java` around lines 269 - 275, createSaveTransaction currently dereferences late-bound providers unconditionally (chunkProvider.get(), networkSystem.get()) causing NPEs for explicit saves during transitions; update createSaveTransaction to guard those calls by checking provider presence (e.g. chunkProvider.isPresent()/networkSystem.isPresent() or equivalent) before calling addChunksToSaveTransaction/addPlayersToSaveTransaction, and if a provider is absent either skip adding that data to the SaveTransaction or return a clear failure/exception; reference methods: createSaveTransaction, addChunksToSaveTransaction, addPlayersToSaveTransaction and the callers waitForCompletionOfPreviousSaveAndStartSaving / startSaving so the fix prevents NPEs during explicit saves.engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java (1)
185-191:⚠️ Potential issue | 🔴 Critical
CoreRegistryis being cleared here, not initialized.At Line 191,
rootContextis stillnull; it is not created until Line 262. That leavesCoreRegistryunset through the rest of the constructor and early initialization, which breaks the exact compatibility window the comment says is still needed for loaders that read fromCoreRegistryduring engine bring-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java` around lines 185 - 191, In the TerasologyEngine constructor the CoreRegistry is being set with a null rootContext; move or perform the rootContext initialization before calling CoreRegistry.setContext so CoreRegistry is populated for early asset/load-time users. Specifically, ensure rootContext is constructed (the same object later created around the current rootContext creation site) prior to invoking CoreRegistry.setContext(rootContext) in the TerasologyEngine constructor (or alternatively defer the CoreRegistry.setContext call to immediately after the rootContext is created), so loaders that access CoreRegistry during engine bring-up see a valid context.
♻️ Duplicate comments (2)
engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseBlocks.java (1)
27-28:⚠️ Potential issue | 🟠 MajorAvoid the concrete
BlockManagerImplcast here.If a different
BlockManagerimplementation is ever bound, Line 28 fails withClassCastExceptionduring load. Prefer requesting the concrete type directly or exposing this initialization hook through a service contract that does not require a down-cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseBlocks.java` around lines 27 - 28, InitialiseBlocks.step() is casting context.get(BlockManager.class) to BlockManagerImpl which can cause ClassCastException; instead add an initialization method to the BlockManager interface (e.g., initialise(List<BlockFamily> families, Map<String,Integer> idMap)) and implement it in BlockManagerImpl, then call context.get(BlockManager.class).initialise(gameManifest.getRegisteredBlockFamilies(), gameManifest.getBlockIdMap()) from step() so no down-cast is required.engine/src/main/java/org/terasology/engine/core/module/ModuleManager.java (1)
361-369:⚠️ Potential issue | 🟠 MajorThe new overloads promise
Context, but onlyContextImplworks.Lines 391-393 immediately downcast the public
Contextparameter. Any caller passing anotherContextimplementation will crash with aClassCastExceptionduring module loading.🛠️ Minimal fix
- public void resolveAndLoadEnvironment(Context context, Name... modules) { + public void resolveAndLoadEnvironment(ContextImpl context, Name... modules) { resolveAndLoadEnvironment(context, Arrays.asList(modules)); } @@ - public void resolveAndLoadEnvironment(Context context, Iterable<Name> modules) { + public void resolveAndLoadEnvironment(ContextImpl context, Iterable<Name> modules) { DependencyResolver resolver = new DependencyResolver(registry); @@ - public ModuleEnvironment loadEnvironment(Context context, Set<Module> modules, boolean asPrimary) { + public ModuleEnvironment loadEnvironment(ContextImpl context, Set<Module> modules, boolean asPrimary) { @@ - newEnvironment = new ModuleEnvironment(((ContextImpl) context).getBeanContext(), finalModules, wrappingPermissionProviderFactory); + newEnvironment = new ModuleEnvironment(context.getBeanContext(), finalModules, wrappingPermissionProviderFactory); } else { - newEnvironment = new ModuleEnvironment(((ContextImpl) context).getBeanContext(), finalModules, permissionProviderFactory); + newEnvironment = new ModuleEnvironment(context.getBeanContext(), finalModules, permissionProviderFactory); }Also applies to: 385-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/module/ModuleManager.java` around lines 361 - 369, The public overloads of resolveAndLoadEnvironment accept Context but immediately downcast to ContextImpl, risking ClassCastException for other Context implementations; update resolveAndLoadEnvironment(Context context, Iterable<Name> modules) to avoid the blind cast by checking if (context instanceof ContextImpl) and using that instance, otherwise create/wrap a ContextImpl from the provided Context (e.g., new ContextImpl(context) or otherwise copy/bridge the needed bindings) and use the resulting ContextImpl for module loading; adjust any internal calls that rely on ContextImpl so they work with the wrapped instance to preserve the public API promise.
🟡 Minor comments (10)
engine-tests/src/test/java/org/terasology/math/IntMathTest.java-29-31 (1)
29-31:⚠️ Potential issue | 🟡 MinorInitialize
Configbefore putting it inCoreRegistry.At Line 29,
new Config()is registered immediately, butConfigremains uninitialized untilloadDefaults()/load()runs. That can cause null-backed config reads in shared test setup paths.Suggested patch
`@BeforeAll` public static void setUpClass() throws Exception { Config config = new Config(); + config.loadDefaults(); CoreRegistry.setContext(new ContextImpl()); CoreRegistry.put(Config.class, config); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/math/IntMathTest.java` around lines 29 - 31, Config is being constructed and placed into CoreRegistry immediately (via new Config(), CoreRegistry.setContext(ContextImpl), CoreRegistry.put(Config.class, config)) before it's initialized, which can lead to null-backed reads; fix by initializing the Config instance (call config.loadDefaults() or config.load() as appropriate) before registering it in CoreRegistry so any consumers retrieve a fully initialized Config; update the code around the Config creation and registration (the Config variable, its loadDefaults()/load() call, and the CoreRegistry.put(Config.class, config)) accordingly.engine/src/main/java/org/terasology/engine/world/internal/EntityAwareWorldProvider.java-80-83 (1)
80-83:⚠️ Potential issue | 🟡 MinorPrefer a strongly typed constructor dependency over runtime downcast.
Casting
EntityManagertoEngineEntityManagerhere can fail at runtime if DI binding changes. AcceptEngineEntityManagerdirectly (or guard and fail fast with a clear message).Proposed refactor
- public EntityAwareWorldProvider(WorldProviderCore base, EntityManager entityManager, ComponentSystemManager componentSystemManager) { + public EntityAwareWorldProvider(WorldProviderCore base, EngineEntityManager entityManager, ComponentSystemManager componentSystemManager) { super(base); - this.entityManager = (EngineEntityManager) entityManager; + this.entityManager = entityManager; componentSystemManager.register(getTime()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/world/internal/EntityAwareWorldProvider.java` around lines 80 - 83, The constructor EntityAwareWorldProvider currently accepts an EntityManager and downcasts it to EngineEntityManager which can fail at runtime; modify the constructor to accept EngineEntityManager directly (replace parameter EntityManager entityManager with EngineEntityManager entityManager) and assign it to this.entityManager, or if changing the signature is not possible, add a runtime guard in the EntityAwareWorldProvider constructor that checks if (entityManager instanceof EngineEntityManager) before casting and throw an explicit IllegalArgumentException with a clear message if not; update all callers to pass EngineEntityManager where you change the signature.engine-tests/src/test/java/org/terasology/engine/rendering/nui/widgets/treeView/JsonTreeConverterTest.java-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorUse
URL#toURI()/Paths.get(...)instead ofURLDecoderfor classpath resource paths.
URLDecoder.decode(...getFile(), Charset.defaultCharset())misuses the API (designed for form-encoded data, not URL paths) and introduces platform-dependent behavior. Prefer URI-based conversion as already used elsewhere in the codebase (e.g.,ModuleManager.java:434,ClasspathCompromisingModuleFactory.java:151).💡 Proposed fix
+import java.net.URISyntaxException; +import java.net.URL; +import java.nio.file.Paths; +import static org.junit.jupiter.api.Assertions.assertNotNull; ... - File file = new File(URLDecoder.decode( - getClass().getClassLoader().getResource("jsonTreeConverterInput.json").getFile(), Charset.defaultCharset())); + URL resource = getClass().getClassLoader().getResource("jsonTreeConverterInput.json"); + assertNotNull(resource, "Missing test resource jsonTreeConverterInput.json"); + File file; + try { + file = Paths.get(resource.toURI()).toFile(); + } catch (URISyntaxException e) { + fail("Invalid test resource URI"); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/rendering/nui/widgets/treeView/JsonTreeConverterTest.java` around lines 23 - 24, The test currently constructs the File using URLDecoder on getResource(...).getFile(), which is incorrect; update JsonTreeConverterTest to obtain the resource URL, convert it to a URI via getClass().getClassLoader().getResource("jsonTreeConverterInput.json").toURI() (or call toURI() on the URL returned) and then create the File/Path using Paths.get(uri) or new File(uri) instead of URLDecoder; replace the existing File construction (the variable named file) with the URI/Paths-based approach to handle classpath resource paths correctly and platform-independently.engine/src/main/java/org/terasology/engine/recording/RecordingClasses.java-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorCopyright year appears to be in the future.
The copyright year "2026" seems incorrect. Consider updating it to the current year (2025).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/recording/RecordingClasses.java` around lines 1 - 2, Update the copyright header in RecordingClasses (top of the file) to use the correct year: replace "2026" with "2025" so the header reads "Copyright 2025 The Terasology Foundation" while leaving the SPDX identifier unchanged.engine/src/main/java/org/terasology/engine/core/subsystem/EngineSubsystem.java-23-27 (1)
23-27:⚠️ Potential issue | 🟡 MinorUpdate the lifecycle contract text to talk about the registry.
preInitialise(...)andinitialise(...)now receive aServiceRegistry, but the Javadoc still tells implementors to add things into the root context. During this migration that wording is easy to misread as mutatingContextbefore it exists.📝 Proposed doc fix
- * This is an opportunity to add anything into the root context that will carry across the entire run of the engine, - * and may be used by other systems. + * This is an opportunity to register services that will be available from the root context for the entire run + * of the engine and may be used by other systems. @@ - * `@param` serviceRegistry the service registry used to create the root context that will carry across the entire run of the engine. + * `@param` serviceRegistry the service registry used to build the root context for the entire run of the engine. @@ - * `@param` serviceRegistry The service registry used to create the context that will carry across the entire run of the engine. + * `@param` serviceRegistry The service registry used to build the context that will carry across the entire run of the engine.Also applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/subsystem/EngineSubsystem.java` around lines 23 - 27, The Javadoc incorrectly instructs implementors of EngineSubsystem methods preInitialise(...) and initialise(...) to add things into the "root context" even though those methods now receive a ServiceRegistry and the root Context may not yet exist; update the lifecycle contract wording in EngineSubsystem (and corresponding Javadoc blocks for preInitialise and initialise) to state that these methods receive a ServiceRegistry to register services and that implementors should register services via the provided ServiceRegistry rather than mutating a root Context, and clarify when (or where) the root Context becomes available for use.docs/EngineStartUp/Engine-States.md-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorDon't reuse
LoadingStepUpdateas both a node and a subgraph.Line 23 creates a node named
LoadingStepUpdate, then Line 24 reuses the same ID for a subgraph. In Mermaid, node IDs and subgraph IDs share the same namespace and must be unique across the diagram. This ID collision will cause rendering errors. Move the label to the subgraph and keep a single identifier.📝 Proposed fix
- LoadingNUIManagerUpdate--> LoadingStepUpdate[Step Update] - subgraph LoadingStepUpdate + LoadingNUIManagerUpdate --> LoadingStepUpdate + subgraph LoadingStepUpdate[Step Update]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/EngineStartUp/Engine-States.md` around lines 23 - 24, The Mermaid diagram reuses the identifier LoadingStepUpdate as both a node and a subgraph causing an ID collision; fix it by keeping a single unique identifier for the subgraph (or node) and moving the label to the subgraph instead of creating a separate node. Update the connection from LoadingNUIManagerUpdate-->LoadingStepUpdate to point at the subgraph ID you choose (or to a distinct node ID), and remove or rename the duplicate node declaration so LoadingStepUpdate appears only once as the subgraph label/ID.engine/src/main/java/org/terasology/engine/world/block/internal/BlockBuilder.java-40-42 (1)
40-42:⚠️ Potential issue | 🟡 MinorUnchecked
Optional.get()calls may throw if assets are missing.The
getAsset(...).get()calls assume the assets always exist. While these are core engine shapes that should be present, a missing asset would cause aNoSuchElementExceptionwith no indication of which shape failed to load.🛡️ Proposed defensive approach with clearer error messages
- cubeShape = assetManager.getAsset("engine:cube", BlockShape.class).get(); - lowShape = assetManager.getAsset("engine:trimmedLoweredCube", BlockShape.class).get(); - topShape = assetManager.getAsset("engine:trimmedRaisedCube", BlockShape.class).get(); + cubeShape = assetManager.getAsset("engine:cube", BlockShape.class) + .orElseThrow(() -> new IllegalStateException("Missing required asset: engine:cube")); + lowShape = assetManager.getAsset("engine:trimmedLoweredCube", BlockShape.class) + .orElseThrow(() -> new IllegalStateException("Missing required asset: engine:trimmedLoweredCube")); + topShape = assetManager.getAsset("engine:trimmedRaisedCube", BlockShape.class) + .orElseThrow(() -> new IllegalStateException("Missing required asset: engine:trimmedRaisedCube"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/world/block/internal/BlockBuilder.java` around lines 40 - 42, Replace the unchecked Optional.get() calls for cubeShape, lowShape and topShape in BlockBuilder with safe lookups: call assetManager.getAsset(..., BlockShape.class) and either check isPresent() before get() or use orElseThrow to raise a clear exception that names the missing asset (e.g., include "engine:cube", "engine:trimmedLoweredCube", "engine:trimmedRaisedCube") and the BlockBuilder context; this ensures missing core shapes produce a descriptive error instead of a NoSuchElementException from Optional.get().engine/src/main/java/org/terasology/engine/physics/engine/PhysicsEngineManager.java-27-39 (1)
27-39:⚠️ Potential issue | 🟡 MinorRemove or document the unused
getNewPhysicsEnginemethod.The method
getNewPhysicsEngineat line 37 has no usages anywhere in the codebase. OnlyregisterPhysicsEngineis called (inInitialisePhysics), makinggetNewPhysicsEnginedead code. Either remove it or, if it's intended as a public API for external/plugin use, add a comment explicitly stating that and explaining when it should be used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/physics/engine/PhysicsEngineManager.java` around lines 27 - 39, The method getNewPhysicsEngine currently has no usages and appears dead; either remove the method entirely or document it as a public API: if removing, delete the getNewPhysicsEngine(Context) method and any associated Javadoc; if keeping for external/plugin use, add a clear Javadoc to getNewPhysicsEngine explaining it is a public factory for creating a new PhysicsEngine (returns new BulletPhysics(context.get(BlockEntityRegistry.class))) and when/why external callers should use it, and add a `@Deprecated` or `@PublicApi` annotation if applicable; ensure registerPhysicsEngine and BulletPhysics/PhysicsEngine are referenced in the comment to make the intent clear.docs/EngineStartUp/Engine-Start-Up.md-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorRemove speculative wording from the checked-in architecture doc.
Line 24 makes the page non-authoritative even though it lives under
docs/. Please rewrite this as a code-backed description or move the speculation to the PR discussion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/EngineStartUp/Engine-Start-Up.md` around lines 23 - 24, Remove the speculative sentence under the "Why three phases?" heading and replace it with an authoritative, code-backed explanation of the three phases (or move the speculation into the PR discussion); specifically edit the "Why three phases?" section to delete the italicized text "_These are just my guesses based on the provided documentation._" and add a concise, factual description that references the actual implementation or design artifacts (e.g., phase names, responsible modules, function/operation names that implement each phase) so the doc is authoritative rather than speculative.engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java-346-403 (1)
346-403:⚠️ Potential issue | 🟡 MinorInclude the post-load work in
maxProgress.These two steps enqueue most of the remaining load processes after
maxProgresshas already been summed ininit(), while both steps advertise zero cost themselves. That makesprogressValueinaccurate and lets the progress bar jump past 100% during the new phased startup.Also applies to: 406-459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java` around lines 346 - 403, The AddHostPostLoadProcessesStep (and the analogous AddClientPostLoadProcessesStep) currently advertise zero cost/progress while enqueuing many loadProcesses, so update their getExpectedCost() to return the total expected cost of the processes they add (compute deterministically the same list you build in step() — account for RegisterInputSystem when !headless, StartServer only when netMode.isServer() and whether DEDICATED_SERVER vs LISTEN_SERVER, SetupLocalPlayer/AwaitCharacterSpawn when netMode.hasLocalClient(), etc.) and update getProgress() to reflect whether this enqueue step is complete (0 before step() runs, 1 after) or to report proportionally if you prefer fractional progress; ensure the calculation uses the same inputs (netMode, headless, gameManifest, context) and references the same loadProcesses additions so maxProgress includes the enqueued work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20d14ce4-803d-4797-92d2-d0bb36e464dc
📒 Files selected for processing (137)
docs/EngineStartUp/Engine-Start-Up.mddocs/EngineStartUp/Engine-States.mdengine-tests/src/main/java/org/terasology/engine/Environment.javaengine-tests/src/main/java/org/terasology/engine/HeadlessEnvironment.javaengine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.javaengine-tests/src/main/java/org/terasology/engine/integrationenvironment/IntegrationEnvironmentSubsystem.javaengine-tests/src/main/java/org/terasology/engine/testUtil/ModuleManagerFactory.javaengine-tests/src/main/java/org/terasology/engine/testUtil/WithUnittestModule.javaengine-tests/src/main/java/org/terasology/unittest/stubs/StubSubsystem.javaengine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.javaengine-tests/src/test/java/org/terasology/engine/config/flexible/AutoConfigManagerTest.javaengine-tests/src/test/java/org/terasology/engine/core/subsystem/config/BindsSubsystemTest.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/BaseEntityRefTest.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/OwnershipHelperTest.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityManagerTest.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityPoolTest.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/PrefabTest.javaengine-tests/src/test/java/org/terasology/engine/integrationenvironment/CustomSubsystemTest.javaengine-tests/src/test/java/org/terasology/engine/network/internal/NetworkOwnershipTest.javaengine-tests/src/test/java/org/terasology/engine/persistence/ComponentSerializerTest.javaengine-tests/src/test/java/org/terasology/engine/persistence/EntitySerializerTest.javaengine-tests/src/test/java/org/terasology/engine/persistence/internal/StorageManagerTest.javaengine-tests/src/test/java/org/terasology/engine/rendering/nui/editor/ContextMenuUtilsTest.javaengine-tests/src/test/java/org/terasology/engine/rendering/nui/layers/mainMenu/savedGames/GameProviderTest.javaengine-tests/src/test/java/org/terasology/engine/rendering/nui/widgets/treeView/JsonTreeConverterTest.javaengine-tests/src/test/java/org/terasology/engine/world/generator/internal/WorldGeneratorManagerTest.javaengine-tests/src/test/java/org/terasology/math/IntMathTest.javaengine/src/main/java/org/terasology/engine/config/Config.javaengine/src/main/java/org/terasology/engine/config/InputConfig.javaengine/src/main/java/org/terasology/engine/config/flexible/AutoConfigManager.javaengine/src/main/java/org/terasology/engine/context/internal/ContextImpl.javaengine/src/main/java/org/terasology/engine/context/internal/ImmutableContextImpl.javaengine/src/main/java/org/terasology/engine/core/ComponentSystemManager.javaengine/src/main/java/org/terasology/engine/core/GameEngine.javaengine/src/main/java/org/terasology/engine/core/TerasologyEngine.javaengine/src/main/java/org/terasology/engine/core/bootstrap/ClassMetaLibraryImpl.javaengine/src/main/java/org/terasology/engine/core/bootstrap/EntitySystemSetupUtil.javaengine/src/main/java/org/terasology/engine/core/bootstrap/EnvironmentSwitchHandler.javaengine/src/main/java/org/terasology/engine/core/modes/AbstractState.javaengine/src/main/java/org/terasology/engine/core/modes/StateLoading.javaengine/src/main/java/org/terasology/engine/core/modes/StateMainMenu.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/ConfigureEntitySystem.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseBlocks.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseCommandSystem.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseComponentSystemManager.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseEntitySystem.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialisePhysics.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRemoteWorld.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRendering.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseWorld.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/JoinServer.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/LoadExtraBlockData.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterBlocks.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterMods.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterRemoteWorldSystems.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterSystems.javaengine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterWorldSystems.javaengine/src/main/java/org/terasology/engine/core/module/ExternalApiWhitelist.javaengine/src/main/java/org/terasology/engine/core/module/ModuleManager.javaengine/src/main/java/org/terasology/engine/core/subsystem/EngineSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/RenderingSubsystemFactory.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/CommandSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/ConfigurationSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/GameSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/MonitoringSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/NetworkSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/PhysicsSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/TelemetrySubSystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/WorldGenerationSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/common/hibernation/HibernationSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/config/BindsSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessAudio.javaengine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessGraphics.javaengine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessInput.javaengine/src/main/java/org/terasology/engine/core/subsystem/headless/HeadlessTimer.javaengine/src/main/java/org/terasology/engine/core/subsystem/headless/mode/StateHeadlessSetup.javaengine/src/main/java/org/terasology/engine/core/subsystem/headless/renderer/HeadlessRenderingSubsystemFactory.javaengine/src/main/java/org/terasology/engine/core/subsystem/headless/renderer/HeadlessWorldRenderer.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/BaseLwjglSubsystem.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglAudio.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglDisplayDevice.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglGraphics.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglGraphicsManager.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglInput.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglRenderingSubsystemFactory.javaengine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/LwjglTimer.javaengine/src/main/java/org/terasology/engine/entitySystem/entity/internal/PojoEntityManager.javaengine/src/main/java/org/terasology/engine/entitySystem/event/internal/EventSystemImpl.javaengine/src/main/java/org/terasology/engine/entitySystem/metadata/ComponentLibrary.javaengine/src/main/java/org/terasology/engine/entitySystem/metadata/EntitySystemLibrary.javaengine/src/main/java/org/terasology/engine/entitySystem/metadata/EventLibrary.javaengine/src/main/java/org/terasology/engine/entitySystem/prefab/internal/PojoPrefabManager.javaengine/src/main/java/org/terasology/engine/i18n/I18nSubsystem.javaengine/src/main/java/org/terasology/engine/i18n/TranslationSystemImpl.javaengine/src/main/java/org/terasology/engine/identity/storageServiceClient/StorageServiceWorker.javaengine/src/main/java/org/terasology/engine/logic/console/ConsoleImpl.javaengine/src/main/java/org/terasology/engine/network/internal/NetworkSystemImpl.javaengine/src/main/java/org/terasology/engine/network/internal/ServerConnectListManager.javaengine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.javaengine/src/main/java/org/terasology/engine/persistence/typeHandling/TypeHandlerLibraryImpl.javaengine/src/main/java/org/terasology/engine/physics/bullet/BulletPhysics.javaengine/src/main/java/org/terasology/engine/physics/engine/PhysicsEngineManager.javaengine/src/main/java/org/terasology/engine/recording/EventCatcher.javaengine/src/main/java/org/terasology/engine/recording/EventSystemReplayImpl.javaengine/src/main/java/org/terasology/engine/recording/RecordAndReplaySerializer.javaengine/src/main/java/org/terasology/engine/recording/RecordingClasses.javaengine/src/main/java/org/terasology/engine/rendering/backdrop/Skysphere.javaengine/src/main/java/org/terasology/engine/rendering/cameras/PerspectiveCamera.javaengine/src/main/java/org/terasology/engine/rendering/dag/RenderGraph.javaengine/src/main/java/org/terasology/engine/rendering/nui/internal/LwjglCanvasRenderer.javaengine/src/main/java/org/terasology/engine/rendering/nui/internal/NUIManagerInternal.javaengine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/UniverseSetupScreen.javaengine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/advancedGameSetupScreen/AdvancedGameSetupScreen.javaengine/src/main/java/org/terasology/engine/rendering/opengl/ScreenGrabber.javaengine/src/main/java/org/terasology/engine/rendering/opengl/fbms/DisplayResolutionDependentFbo.javaengine/src/main/java/org/terasology/engine/rendering/world/RenderQueuesHelper.javaengine/src/main/java/org/terasology/engine/rendering/world/RenderableWorldImpl.javaengine/src/main/java/org/terasology/engine/rendering/world/WorldRenderer.javaengine/src/main/java/org/terasology/engine/rendering/world/WorldRendererImpl.javaengine/src/main/java/org/terasology/engine/rendering/world/viewDistance/ClientViewDistanceSystem.javaengine/src/main/java/org/terasology/engine/world/block/family/BlockFamilyLibrary.javaengine/src/main/java/org/terasology/engine/world/block/internal/BlockBuilder.javaengine/src/main/java/org/terasology/engine/world/block/internal/BlockManagerImpl.javaengine/src/main/java/org/terasology/engine/world/chunks/blockdata/ExtraBlockDataManager.javaengine/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.javaengine/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/RelevanceSystem.javaengine/src/main/java/org/terasology/engine/world/chunks/remoteChunkProvider/RemoteChunkProvider.javaengine/src/main/java/org/terasology/engine/world/generator/internal/WorldGeneratorManager.javaengine/src/main/java/org/terasology/engine/world/generator/plugin/DefaultWorldGeneratorPluginLibrary.javaengine/src/main/java/org/terasology/engine/world/internal/AbstractWorldProviderDecorator.javaengine/src/main/java/org/terasology/engine/world/internal/EntityAwareWorldProvider.javaengine/src/main/java/org/terasology/engine/world/internal/WorldProviderCoreImpl.javaengine/src/main/java/org/terasology/engine/world/internal/WorldProviderWrapper.javaengine/src/main/java/org/terasology/engine/world/sun/BasicCelestialModel.javaengine/src/main/java/org/terasology/engine/world/sun/DefaultCelestialSystem.javasubsystems/DiscordRPC/src/main/java/org/terasology/subsystem/discordrpc/DiscordRPCSubSystem.javasubsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java
Comment on lines
50
to
55
| protected void reset(Set<Name> moduleNames) throws IOException { | ||
| this.context = new ContextImpl(); | ||
| ServiceRegistry serviceRegistry = new ServiceRegistry(); | ||
| RecordAndReplayCurrentStatus recordAndReplayCurrentStatus = new RecordAndReplayCurrentStatus(); | ||
| context.put(RecordAndReplayCurrentStatus.class, recordAndReplayCurrentStatus); | ||
| serviceRegistry.with(RecordAndReplayCurrentStatus.class).lifetime(Lifetime.Singleton).use(() -> recordAndReplayCurrentStatus); | ||
| CoreRegistry.setContext(context); |
There was a problem hiding this comment.
Don't swap context halfway through reset().
Line 55 publishes the first ContextImpl to CoreRegistry, then Line 89 replaces this.context with a different service-backed instance. Anything setup hooks put into the first context is lost, and any remaining CoreRegistry callers keep resolving from the stale one. That makes startup behavior depend on which access path a caller uses.
Also applies to: 73-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine-tests/src/main/java/org/terasology/engine/Environment.java` around
lines 50 - 55, reset() currently publishes a ContextImpl to CoreRegistry early
and then replaces this.context with a different service-backed instance later,
losing any setup performed on the first context; fix this by building the final
service-backed ContextImpl (using ServiceRegistry and registering
RecordAndReplayCurrentStatus and other services) before assigning this.context
and calling CoreRegistry.setContext(context) so CoreRegistry always receives the
fully wired context (adjust construction code around ServiceRegistry, the
RecordAndReplayCurrentStatus registration, and the this.context assignment in
reset() — also apply the same change to the code in the 73-93 block).
Comment on lines
+141
to
+142
| serviceRegistry.with(BlockFamilyLibrary.class).lifetime(Lifetime.Singleton).use(BlockFamilyLibrary.class); | ||
| serviceRegistry.with(StorageManager.class).lifetime(Lifetime.Singleton).use(ReadWriteStorageManager.class); |
There was a problem hiding this comment.
Register the same BlockFamilyLibrary that the block manager uses.
Lines 141-142 create a fresh BlockFamilyLibrary, while Lines 171-175 create a separate BlockManagerImpl. Consumers injected with BlockFamilyLibrary will not see the families loaded into the block manager, so persistence code can end up serializing against a different registry than the world uses.
Also applies to: 171-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine-tests/src/main/java/org/terasology/engine/HeadlessEnvironment.java`
around lines 141 - 142, The test environment currently registers a standalone
BlockFamilyLibrary, causing a different registry than the BlockManagerImpl;
instead instantiate BlockManagerImpl (the same one created later) and register
its BlockFamilyLibrary instance under BlockFamilyLibrary so all consumers share
the same registry: create the BlockManagerImpl (as in the block of code that
constructs BlockManagerImpl) first, register that BlockManagerImpl with
serviceRegistry, then call
serviceRegistry.with(BlockFamilyLibrary.class).use(blockManager.getBlockFamilyLibrary())
(or equivalent getter) so the BlockFamilyLibrary binding references the
BlockManagerImpl's library rather than a fresh instance.
Comment on lines
+43
to
+45
| public void initialise(GameEngine engine, ServiceRegistry serviceRegistry) { | ||
| EngineSubsystem.super.initialise(engine, serviceRegistry); | ||
| registerUnittestModule(moduleManager); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
MM=$(fd 'ModuleManager.java' | head -n1)
if [ -z "$MM" ]; then
echo "ModuleManager.java not found" >&2
exit 1
fi
echo "resolveAndLoadEnvironment overloads / implementation:"
rg -n -C8 'resolveAndLoadEnvironment\s*\(' "$MM"
echo
echo "Current call sites:"
rg -n -C3 'resolveAndLoadEnvironment\s*\(' --type=javaRepository: MovingBlocks/Terasology
Length of output: 6380
🏁 Script executed:
cat -n engine/src/main/java/org/terasology/engine/core/module/ModuleManager.java | sed -n '369,450p'Repository: MovingBlocks/Terasology
Length of output: 3596
The empty context will cause the unittest module to load without services available in its BeanContext.
The Context parameter is used: resolveAndLoadEnvironment(Context context, Iterable<Name> modules) passes it to loadEnvironment(), which extracts ((ContextImpl) context).getBeanContext() and passes it to the ModuleEnvironment constructor (lines 391, 393). Passing new ContextImpl() means the unittest module loads with an empty BeanContext, unlike in HeadlessEnvironment.java:290 where a fully-configured context is provided.
Also applies to: 48-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine-tests/src/main/java/org/terasology/engine/testUtil/WithUnittestModule.java`
around lines 43 - 45, The initialise method currently creates a new, empty
ContextImpl before loading the unittest module which results in an empty
BeanContext; instead obtain and pass the fully-configured context used by the
engine so the unittest module gets the proper BeanContext. Change the initialise
implementation (method initialise(...) that calls
registerUnittestModule(moduleManager) and any similar call sites at lines 48-50)
to reuse the engine/serviceRegistry context (the same Context instance used by
resolveAndLoadEnvironment(...) / loadEnvironment(...) / ModuleEnvironment)
rather than new ContextImpl(), mirroring how HeadlessEnvironment provides its
configured Context into ModuleEnvironment; ensure you pass that existing
Context/BeanContext into the module-loading path.
Comment on lines
+79
to
84
| ServiceRegistry serviceRegistry = new ServiceRegistry(); | ||
| serviceRegistry.with(ModuleManager.class).lifetime(Lifetime.Singleton).use(() -> moduleManager); | ||
|
|
||
| mockTime = mock(EngineTime.class); | ||
| context.put(Time.class, mockTime); | ||
| serviceRegistry.with(Time.class).lifetime(Lifetime.Singleton).use(() -> mockTime); | ||
| NetworkSystemImpl networkSystem = new NetworkSystemImpl(mockTime, context); |
There was a problem hiding this comment.
Rebuild the per-test context from the headless root.
context is static, so Line 103 currently wraps whatever the previous test left behind. From the second test onward you get a parent chain of old test contexts, and unresolved lookups can leak stale services/state across cases.
🧪 Suggested fix
`@BeforeEach`
public void setup() throws Exception {
+ Context baseContext = env.getContext();
ServiceRegistry serviceRegistry = new ServiceRegistry();
serviceRegistry.with(ModuleManager.class).lifetime(Lifetime.Singleton).use(() -> moduleManager);
mockTime = mock(EngineTime.class);
serviceRegistry.with(Time.class).lifetime(Lifetime.Singleton).use(() -> mockTime);
- NetworkSystemImpl networkSystem = new NetworkSystemImpl(mockTime, context);
+ NetworkSystemImpl networkSystem = new NetworkSystemImpl(mockTime, baseContext);
serviceRegistry.with(Game.class).lifetime(Lifetime.Singleton).use(Game::new);
serviceRegistry.with(NetworkSystem.class).lifetime(Lifetime.Singleton).use(() -> networkSystem);
EntitySystemSetupUtil.addReflectionBasedLibraries(serviceRegistry);
@@
- context = new ContextImpl(context, serviceRegistry);
+ context = new ContextImpl(baseContext, serviceRegistry);Also applies to: 103-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java`
around lines 79 - 84, TerasologyTestingEnvironment currently reuses the static
context across tests causing parent chains to leak; before creating the
ServiceRegistry and instantiating NetworkSystemImpl you must rebuild a fresh
per-test context from the headless root (do not reuse the static context), e.g.
obtain/create a new child context off the headless root and assign it to the
variable context so subsequent calls to ServiceRegistry.with(...).use(...) and
new NetworkSystemImpl(mockTime, context) operate on a fresh context; update the
setup in TerasologyTestingEnvironment to recreate context per test rather than
reusing the static instance.
| BlockManager mockBlockManager = context.get(BlockManager.class); // 'mock' added to avoid hiding a field | ||
| ExtraBlockDataManager extraDataManager = context.get(ExtraBlockDataManager.class); | ||
| RecordedEventStore recordedEventStore = new RecordedEventStore(); | ||
| serviceRegistry.with(Game.class).lifetime(Lifetime.Singleton).use(Game::new); |
There was a problem hiding this comment.
Register the loaded Game instance.
Lines 97-98 initialize game, but Line 85 still binds Game.class to new Game(). Anything resolved from DI gets an uninitialized Game instead of the loaded test game.
🧩 Suggested fix
- serviceRegistry.with(Game.class).lifetime(Lifetime.Singleton).use(Game::new);
+ Game game = new Game();
+ game.load(new GameManifest("world1", "world1", 0));
+ serviceRegistry.with(Game.class).lifetime(Lifetime.Singleton).use(() -> game);
serviceRegistry.with(NetworkSystem.class).lifetime(Lifetime.Singleton).use(() -> networkSystem);
EntitySystemSetupUtil.addReflectionBasedLibraries(serviceRegistry);
@@
- Game game = new Game();
- game.load(new GameManifest("world1", "world1", 0));
serviceRegistry.with(StorageManager.class).lifetime(Lifetime.Singleton).use(ReadWriteStorageManager.class);Also applies to: 97-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java`
at line 85, The current registration binds Game.class to a new Game()
(serviceRegistry.with(Game.class).lifetime(Lifetime.Singleton).use(Game::new))
which causes DI to supply an uninitialized instance; change the binding to
register the loaded test instance instead by pointing the service registry at
the initialized game variable (the game created in TerasologyTestingEnvironment)
— e.g., after the code that initializes/loads the game, replace the Game::new
registration with a supplier that returns the loaded game instance or use the
registry's instance-binding API so all resolves return that initialized game.
Comment on lines
+60
to
62
| // Allow persistence internals needed during client join (EntityRestorer, etc.) | ||
| .add("org.terasology.engine.persistence.internal") | ||
| .add("org.terasology.math") |
There was a problem hiding this comment.
Avoid whitelisting the full engine.persistence.internal package.
Granting module access to an entire internal namespace broadens the sandbox surface. Prefer allowlisting only the specific required types to keep the boundary tight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine/src/main/java/org/terasology/engine/core/module/ExternalApiWhitelist.java`
around lines 60 - 62, The whitelist currently allows the entire package
"org.terasology.engine.persistence.internal" which is too broad; instead remove
that package-level .add(...) entry in ExternalApiWhitelist and replace it with
individual .add("fully.qualified.ClassName") entries for only the required types
(e.g., org.terasology.engine.persistence.internal.EntityRestorer and any other
specific classes used during client join such as the exact persistence helper
classes), ensuring you only allow the minimal set of fully-qualified class names
needed by modules.
Comment on lines
+79
to
+88
| @Inject | ||
| public PojoEntityManager(Optional<PrefabManager> prefabManager, Optional<EventSystem> eventSystem, | ||
| Optional<EntitySystemLibrary> entitySystemLibrary, Optional<TypeHandlerLibrary> typeSerializerLibrary) { | ||
| this.prefabManager = prefabManager.orElse(null); | ||
| this.eventSystem = eventSystem.orElse(null); | ||
| if (entitySystemLibrary.isPresent()) { | ||
| this.componentLibrary = entitySystemLibrary.get().getComponentLibrary(); | ||
| } | ||
| this.typeSerializerLibrary = typeSerializerLibrary.orElse(null); | ||
| } |
There was a problem hiding this comment.
Don't let DI publish a half-initialized PojoEntityManager.
Only eventSystem is nullable in the rest of this class. Making PrefabManager, EntitySystemLibrary, and TypeHandlerLibrary optional means the container can publish a manager that later NPEs in paths like copy() and copyComponents(). Missing wiring should fail during bootstrap, not after the service is already in use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine/src/main/java/org/terasology/engine/entitySystem/entity/internal/PojoEntityManager.java`
around lines 79 - 88, The constructor for PojoEntityManager currently accepts
Optional<PrefabManager>, Optional<EntitySystemLibrary>, and
Optional<TypeHandlerLibrary>, allowing the container to publish a
half-initialized PojoEntityManager that will NPE later in methods like copy()
and copyComponents(); fix this by failing fast during construction: either
change the constructor to require non-Optional PrefabManager,
EntitySystemLibrary and TypeHandlerLibrary (inject concrete instances) or, if
Optionals must be used, explicitly validate presence inside the
PojoEntityManager constructor and throw a clear runtime exception (e.g.
IllegalStateException) when prefabManager, entitySystemLibrary (and thus
componentLibrary) or typeSerializerLibrary are missing so the DI bootstrap fails
instead of deferring NPEs to copy()/copyComponents(); ensure you reference and
initialize fields prefabManager, componentLibrary and typeSerializerLibrary
accordingly while leaving eventSystem nullable as before.
Comment on lines
509
to
515
| WidgetUtil.trySubscribe(this, "createWorld", button -> { | ||
| context.get(UniverseWrapper.class).setSeed(seed.getText()); | ||
| universeWrapper.setSeed(seed.getText()); | ||
| final UniverseSetupScreen universeSetupScreen = getManager() | ||
| .createScreen(UniverseSetupScreen.ASSET_URI, UniverseSetupScreen.class); | ||
| saveConfiguration(); | ||
| universeSetupScreen.setEnvironment(); | ||
| universeSetupScreen.setEnvironment(universeWrapper); | ||
| triggerForwardAnimation(universeSetupScreen); |
There was a problem hiding this comment.
Initialize/guard universeWrapper before button actions
universeWrapper is dereferenced in both handlers without null checks, so this can crash if setEnvironment(...) was not called first (Line 510, Line 523).
💡 Proposed fix
`@Override`
public void initialise() {
setAnimationSystem(MenuAnimationSystems.createDefaultSwipeAnimation());
+ if (universeWrapper == null) {
+ universeWrapper = new UniverseWrapper();
+ }
remoteModuleRegistryUpdater = Executors.newSingleThreadExecutor(
new ThreadFactoryBuilder() WidgetUtil.trySubscribe(this, "createWorld", button -> {
+ if (universeWrapper == null) {
+ getManager().createScreen(MessagePopup.ASSET_URI, MessagePopup.class)
+ .setMessage("Error", "World setup is not initialized.");
+ return;
+ }
universeWrapper.setSeed(seed.getText());
final UniverseSetupScreen universeSetupScreen = getManager()
.createScreen(UniverseSetupScreen.ASSET_URI, UniverseSetupScreen.class);Also applies to: 523-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/advancedGameSetupScreen/AdvancedGameSetupScreen.java`
around lines 509 - 515, The code dereferences universeWrapper in the createWorld
and createCampaign button handlers (within the WidgetUtil.trySubscribe lambdas)
without guarding for null; update both handlers in AdvancedGameSetupScreen to
first ensure universeWrapper is non-null (either initialize it to a default/new
UniverseWrapper instance or check if (universeWrapper == null) and bail
out—e.g., log/show an error and return) before calling
universeWrapper.setSeed(...) and
universeSetupScreen.setEnvironment(universeWrapper) (and the analogous calls in
the other handler); keep the rest of the flow (saveConfiguration(),
createScreen(...), triggerForwardAnimation(...)) unchanged after the
null-check/initialization.
Comment on lines
+146
to
+147
| universeWrapper = new UniverseWrapper(); | ||
|
|
There was a problem hiding this comment.
Use the passed UniverseWrapper as the single source of truth
setEnvironment(...) never assigns the incoming wrapper to this.universeWrapper, while initialise() creates a new one. The screen then reads/writes the field, not the context entry, so state can desynchronize.
💡 Proposed fix
`@Override`
public void initialise() {
setAnimationSystem(MenuAnimationSystems.createDefaultSwipeAnimation());
-
- universeWrapper = new UniverseWrapper();
+ if (universeWrapper == null) {
+ universeWrapper = new UniverseWrapper();
+ } public void setEnvironment(UniverseWrapper universeWrapper) {
+ this.universeWrapper = (universeWrapper != null) ? universeWrapper : new UniverseWrapper();
prepareContext();
@@
- context.put(UniverseWrapper.class, universeWrapper);
+ context.put(UniverseWrapper.class, this.universeWrapper);Also applies to: 385-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/UniverseSetupScreen.java`
around lines 146 - 147, setEnvironment(...) currently doesn't assign the
provided UniverseWrapper and initialise() creates a new UniverseWrapper, causing
desync; update setEnvironment to assign the incoming wrapper to
this.universeWrapper and remove the new UniverseWrapper() allocation in
initialise so the screen always operates on the passed-in UniverseWrapper from
the context; review other methods that reference universeWrapper (e.g., any
logic around lines ~385-399) to ensure they use this.universeWrapper (the
context-provided instance) rather than creating or reading a separate instance.
Comment on lines
+128
to
+153
| ServiceRegistry serviceRegistry = new ServiceRegistry(); | ||
| registerRenderingSupport(serviceRegistry); | ||
| registerRenderGraph(serviceRegistry); | ||
| this.context = new ContextImpl(context, serviceRegistry); | ||
|
|
||
| initRenderGraph(); | ||
| renderGraph = this.context.get(RenderGraph.class); | ||
| } | ||
|
|
||
| @Override | ||
| public void init() { | ||
| initRenderingSupport(); | ||
| initRenderingModules(); | ||
|
|
||
| console = context.get(Console.class); | ||
| MethodCommand.registerAvailable(this, console, context); | ||
| } | ||
|
|
||
| private void initRenderingSupport() { | ||
| ScreenGrabber screenGrabber = new ScreenGrabber(context); | ||
| context.put(ScreenGrabber.class, screenGrabber); | ||
|
|
||
| displayResolutionDependentFbo = new DisplayResolutionDependentFbo( | ||
| context.get(Config.class).getRendering(), screenGrabber, context.get(DisplayDevice.class)); | ||
| context.put(DisplayResolutionDependentFbo.class, displayResolutionDependentFbo); | ||
| private void registerRenderingSupport(ServiceRegistry serviceRegistry) { | ||
| serviceRegistry.with(ScreenGrabber.class).lifetime(Lifetime.Singleton).use(ScreenGrabber.class); | ||
| serviceRegistry.with(DisplayResolutionDependentFbo.class).lifetime(Lifetime.Singleton).use(DisplayResolutionDependentFbo.class); | ||
|
|
||
| shaderManager.initShaders(); | ||
|
|
||
| context.put(WorldRenderer.class, this); | ||
| context.put(RenderQueuesHelper.class, renderQueues); | ||
| context.put(RenderableWorld.class, renderableWorld); | ||
| serviceRegistry.with(RenderQueuesHelper.class).lifetime(Lifetime.Singleton).use(() -> renderQueues); | ||
| serviceRegistry.with(RenderableWorld.class).lifetime(Lifetime.Singleton).use(() -> renderableWorld); | ||
| } |
There was a problem hiding this comment.
Keep ScreenGrabber visible from the shared game context.
ReadWriteStorageManager.saveGamePreviewImage() still resolves ScreenGrabber through CoreRegistry. Registering it only in this renderer's private ContextImpl makes that path miss the live instance, which is the save-preview / black-screenshot regression this PR already notes. Either bind it in the parent registry as well or migrate the save path to inject the same instance from this context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine/src/main/java/org/terasology/engine/rendering/world/WorldRendererImpl.java`
around lines 128 - 153, The ScreenGrabber is only registered in the renderer's
private ServiceRegistry (registerRenderingSupport) and therefore isn't visible
to code resolving via the shared/context root (e.g.,
ReadWriteStorageManager.saveGamePreviewImage via CoreRegistry); make the
ScreenGrabber instance visible in the parent/shared context as well: when you
create/register ScreenGrabber in registerRenderingSupport or after
shaderManager.initShaders(), also bind the same instance into the parent
ContextImpl parent (the original context variable passed into new
ContextImpl(context, serviceRegistry)) so lookup by CoreRegistry/parent context
returns the live ScreenGrabber; reference the ScreenGrabber class and ensure the
same instance used by registerRenderingSupport is put into the parent context
(or register it in the parent registry) instead of only in the renderer-local
ServiceRegistry.
Member
|
Big note: I triggered AI reviews on this solely for (near) future reference - to have a more surmountable challenge I'm going to put a draft set of changes addressing review and other hot topic changes in a separate review PR after merging this PR as-is, if everything goes well. Previously I made https://github.com/MovingBlocks/gestalt/tree/release/v8.x which built a Gestalt 8.0.1 snapshot (merging things to get clean releases right now looks... daunting). I'm not sure if this PR's request of 8.0.0 snapshot might contain enough changes to work, but I guess it must as it tests out locally! Both the latest 8.0.0 and 8.0.1 snapshots look to have built around the same time ... :sus: Locally I've merged this branch into my develop as well as grabbed the MTE changes. I tested plain develop first and noted the multiplayer bug where a 2nd client cannot see inventory contests inside a chest placed by the host player, but they can place chests themselves which they can access. After the dust settles I suspect that won't be a hard fix. I'm working with my new GDD agentic framework which has some extensive utility, local mid-term memory, and skills for review and the like. I'll be absorbing a lot of stuff locally and it may take a little while to get it all processed. Thank you again @BenjaminAmos for sticking with this so long. Record PR for the ages :-) |
Cervator
added a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 29, 2026
setEnvironment() received a UniverseWrapper parameter but never assigned it to the instance field. The screen's UI bindings read from the field, not the context, so seed/generator/server settings from AdvancedGameSetupScreen were silently discarded. Flagged independently by both Copilot and CodeRabbit on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
15 tasks
Cervator
added a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 30, 2026
Remove commented-out world generator initialization code and the now-unused gameManifest field. The world generator setup was moved to RegisterWorldSystems during the DI migration. BSA flagged this on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cervator
added a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 30, 2026
Rename the nonsensical variable name to something descriptive. This is a per-module child context used to inject widget-specific dependencies (TypeWidgetLibrary) during control widget initialization. BSA flagged this on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cervator
added a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 30, 2026
Inject NetworkSystem as an explicit constructor parameter instead of pulling it from Context. Context is retained only for the lazy PermissionManager lookup (provided by @share at runtime, not available at construction time). BSA flagged this on PR MovingBlocks#5299: "This class does not require a full context. Inject NetworkSystem and PermissionManager instead." PermissionManager cannot be fully injected yet due to its lifecycle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cervator
added a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 30, 2026
Replace Context parameter with explicit ReflectFactory and CopyStrategyLibrary parameters in both constructors. The Context was only used to pull these two dependencies, making it an unnecessary intermediary. BSA flagged this on PR MovingBlocks#5299: "We should not be injecting Context directly unless absolutely necessary." Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 31, 2026
…loadEnvironment The loadEnvironment(Context, ...) method requires a ContextImpl to extract the BeanContext for module DI, but the parameter type is the broader Context interface. Added Javadoc documenting this requirement and replaced the blind cast with instanceof + clear error message. BSA flagged this on PR MovingBlocks#5299: "Should this implementation requirement be documented?" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 31, 2026
…ng null The ServiceRegistry supplier silently returned null when AutoConfig construction failed, publishing a null singleton. Later lookups would get null and NPE far from the root cause. Now logs the error and throws RuntimeException to surface the problem immediately. CodeRabbit flagged this on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Mar 31, 2026
setEnvironment() received a UniverseWrapper parameter but never assigned it to the instance field. The screen's UI bindings read from the field, not the context, so seed/generator/server settings from AdvancedGameSetupScreen were silently discarded. Flagged independently by both Copilot and CodeRabbit on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 1, 2026
…serialize Two bugs in deserialize for private fields: 1. findSetter(field) can return null — no null check (NPE risk) 2. setter.invoke(result) passed no value argument — the setter was called with zero args, silently failing to set the field Fixed to match the serialize side's pattern: null-check the setter, log an error if missing, and pass fieldValue.get() as the argument. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, critical). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 1, 2026
…LibraryImpl forModuleEnvironment() creates an instance via the @Inject constructor (which already calls populateWithDefaultHandlers), then called it again, duplicating all default handler/factory registrations. The handlers append to a list without deduplication. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, major). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 1, 2026
…ider Replace EntityManager parameter with EngineEntityManager in the @Inject constructor, eliminating the runtime downcast. Updated the two WorldProviderCoreWorkAround subclasses to match. Gestalt DI auto-maps PojoEntityManager to all its interfaces including EngineEntityManager. CodeRabbit flagged this on PR MovingBlocks#5299 (minor). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 1, 2026
The Javadoc still said "add anything into the root context" when the methods now receive a ServiceRegistry. Updated to say "register services" and "build the root context" to match the DI migration. CodeRabbit flagged this on PR MovingBlocks#5299 (minor). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 tasks
Draft
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
…ctor The constructor called CoreRegistry.setContext(rootContext) before rootContext was created, passing null. The correct setContext call happens in initialize() after rootContext is constructed. Removed the premature call and updated the comment. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, critical). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
mouseWheelUpBind and mouseWheelDownBind can be null after clearBinds() and may not be re-assigned if no wheel bindings are registered. Add null checks before accessing getId(). CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, minor). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
LoadingStepUpdate was used as both a node label and a subgraph ID, which share the same Mermaid namespace. Move the label to the subgraph declaration so the ID is only defined once. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, minor). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
…action ChunkProvider and NetworkSystem providers are dereferenced unconditionally in createSaveTransaction(). A save triggered during a state transition could NPE if the providers are not yet resolved. Guard with null checks and fail with a clear message. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, major). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
…ctor The constructor called CoreRegistry.setContext(rootContext) before rootContext was created, passing null. The correct setContext call happens in initialize() after rootContext is constructed. Removed the premature call and updated the comment. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, critical). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
mouseWheelUpBind and mouseWheelDownBind can be null after clearBinds() and may not be re-assigned if no wheel bindings are registered. Add null checks before accessing getId(). CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, minor). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
LoadingStepUpdate was used as both a node label and a subgraph ID, which share the same Mermaid namespace. Move the label to the subgraph declaration so the ID is only defined once. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, minor). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 3, 2026
…action ChunkProvider and NetworkSystem providers are dereferenced unconditionally in createSaveTransaction(). A save triggered during a state transition could NPE if the providers are not yet resolved. Guard with null checks and fail with a clear message. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, major). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 task
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 5, 2026
Three issues in the client join path: 1. Dead thread was treated as success — if the environment switch threw an exception, the thread died and step() returned true. Now uses FutureTask which surfaces exceptions via get(). 2. Unsafe .stream().findFirst().get() for BeanContext extraction replaced with orElseThrow() and a clear error message. 3. CoreRegistry.setContext() was called from the worker thread. Now the FutureTask returns the Context and the main thread sets CoreRegistry after get() completes successfully. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, critical). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 5, 2026
Add comment clarifying that the world seed is carried by the GameManifest from the UI (via GameManifestProvider), with a random fallback if not specified. BSA asked "Where is the seed being set now?" on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 5, 2026
…uctor Mark the old constructor that resolves dependencies via CoreRegistry as @deprecated. The @Inject constructor is the correct DI path. No callers use the old constructor directly — it exists only as a fallback. BSA flagged this on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 5, 2026
…ages ScreenGrabber was registered only in WorldRendererImpl's private child context, invisible from the shared game context. ReadWriteStorageManager resolves it via CoreRegistry.get(ScreenGrabber.class) when saving the game preview image, which returned null — resulting in black preview screenshots. Propagate the instance to CoreRegistry after init(). CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range, major). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merged
4 tasks
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 5, 2026
Each @beforeeach was wrapping the previous test's context with new ContextImpl(context, serviceRegistry), creating a growing parent chain and leaking services between tests. Introduce a stable baseContext from @BeforeAll and create per-test child contexts from that instead. Also register the loaded Game instance in TerasologyTestingEnvironment instead of an uninitialized Game::new. CodeRabbit flagged this cluster on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 5, 2026
…et() CoreRegistry was set to the initial empty context at line 55 but never updated after the real context was created from the serviceRegistry at line 89. Any CoreRegistry lookup after reset() resolved from the stale empty context. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 tasks
Cervator
added a commit
that referenced
this pull request
Apr 19, 2026
Each @beforeeach was wrapping the previous test's context with new ContextImpl(context, serviceRegistry), creating a growing parent chain and leaking services between tests. Introduce a stable baseContext from @BeforeAll and create per-test child contexts from that instead. Also register the loaded Game instance in TerasologyTestingEnvironment instead of an uninitialized Game::new. CodeRabbit flagged this cluster on PR #5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cervator
added a commit
that referenced
this pull request
Apr 19, 2026
…et() CoreRegistry was set to the initial empty context at line 55 but never updated after the real context was created from the serviceRegistry at line 89. Any CoreRegistry lookup after reset() resolved from the stale empty context. CodeRabbit flagged this on PR #5299 (outside-diff-range). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contains
This pull request migrates the majority of Terasology to actually use Gestalt DI. It significantly refactors and re-architects the fundamentals of the application, making a concerted effort to finally eliminate
CoreRegistryonce and for all.This is a wide-ranging refactor that affects essentially the entire game, particularly focused on engine bring-up and reducing the various inter-dependencies between the tasks performed there. It accomplishes this by migrating the the carefully-ordered brittle start-up sequence to instead be based around a well-ordered dependency injection model (via gestalt-di), split into three distinct phases (pre-init, init and post-init).
Along with plenty else that I am unable to recall since this has been near untouched for months. There are some notes on architecture that I wrote included in the
docs/EngineStartUpfolder but I cannot be sure of how closely they match the current state of the code at this time.How to test
Absolutely everything needs to be tested again. No code is safe from this refactor.
Getting all the tests passing would be a good first step.It is also rather hard to test anything apart from the unit tests until the input issue is fixed.Excluding the known issues, testing that you can create a new game and then run it would be useful. Likewise, loading a saved game from a clean state is also important. It is critical that everything from the
iotaline-up works as before. Other modules (such as theJoshariasSurvivalline-up) should work but are not as thoroughly tested.Known Issues
develop)Outstanding before merging
Testing everything(this is unrealistic - everything tested so-far apart from the known issues seems to work however)develop?)Summary by CodeRabbit
Documentation
Improvements
Bug Fixes