Skip to content

review: fix deserialize bugs, duplicate handlers, type safety, and Javadoc #5318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Cervator merged 5 commits into from
Apr 2, 2026
Merged

review: fix deserialize bugs, duplicate handlers, type safety, and Javadoc #5318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ed6da9
fix: null-check setter and pass value in ObjectFieldMapTypeHandler.de…
Cervator Apr 1, 2026
e7ddacc
fix: remove duplicate populateWithDefaultHandlers call in TypeHandler…
Cervator Apr 1, 2026
5fadcc3
refactor: accept EngineEntityManager directly in EntityAwareWorldProv…
Cervator Apr 1, 2026
617c252
docs: update EngineSubsystem lifecycle Javadoc for ServiceRegistry
Cervator Apr 1, 2026
2ed7b37
fix: remove unused EntityManager imports, align Javadoc style
Cervator Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...e/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRemoteWorld.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import org.terasology.engine.core.TerasologyConstants;
import org.terasology.engine.core.modes.SingleStepLoadProcess;
import org.terasology.engine.core.subsystem.RenderingSubsystemFactory;
import org.terasology.engine.entitySystem.entity.EntityManager;
import org.terasology.engine.entitySystem.entity.internal.EngineEntityManager;
import org.terasology.engine.game.GameManifest;
import org.terasology.engine.logic.players.LocalPlayer;
import org.terasology.engine.recording.DirectionAndOriginPosRecorderList;
Expand Down Expand Up @@ -91,7 +91,7 @@ public static final class WorldProviderCoreWorkAround extends EntityAwareWorldPr
implements WorldProviderCore, BlockEntityRegistry {
@Inject
public WorldProviderCoreWorkAround(WorldInfo info, ChunkProvider chunkProvider, BlockManager blockManager,
EntityManager entityManager, ComponentSystemManager componentSystemManager) {
EngineEntityManager entityManager, ComponentSystemManager componentSystemManager) {
super(new WorldProviderCoreImpl(info, chunkProvider, blockManager, entityManager), entityManager, componentSystemManager);
}
}
Expand Down
4 changes: 2 additions & 2 deletions engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseWorld.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import org.terasology.engine.core.modes.SingleStepLoadProcess;
import org.terasology.engine.core.modes.StateMainMenu;
import org.terasology.engine.core.subsystem.RenderingSubsystemFactory;
import org.terasology.engine.entitySystem.entity.EntityManager;
import org.terasology.engine.entitySystem.entity.internal.EngineEntityManager;
import org.terasology.engine.game.GameManifest;
import org.terasology.engine.logic.players.LocalPlayer;
import org.terasology.engine.persistence.StorageManager;
Expand Down Expand Up @@ -160,7 +160,7 @@ public static final class WorldProviderCoreWorkAround extends EntityAwareWorldPr
implements WorldProviderCore, BlockEntityRegistry {
@Inject
public WorldProviderCoreWorkAround(WorldInfo info, ChunkProvider chunkProvider, BlockManager blockManager,
EntityManager entityManager, ComponentSystemManager componentSystemManager) {
EngineEntityManager entityManager, ComponentSystemManager componentSystemManager) {
super(new WorldProviderCoreImpl(info, chunkProvider, blockManager, entityManager), entityManager, componentSystemManager);
}
}
Expand Down
12 changes: 6 additions & 6 deletions engine/src/main/java/org/terasology/engine/core/subsystem/EngineSubsystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,19 @@ public interface EngineSubsystem {

/**
* Called on each system before initialisation.
* 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
*/
default void preInitialise(ServiceRegistry serviceRegistry) {
}

/**
* Called to initialise the system
* Called to initialise the system.
*
* @param engine The game engine
* @param serviceRegistry The service registry used to create the context that will carry across the entire run of the engine.
* @param engine The game engine
* @param serviceRegistry The service registry used to build the context for the entire run of the engine
*/
default void initialise(GameEngine engine, ServiceRegistry serviceRegistry) {
}
Expand Down
7 changes: 2 additions & 5 deletions .../src/main/java/org/terasology/engine/persistence/typeHandling/TypeHandlerLibraryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,8 @@ public static TypeHandlerLibrary withReflections(Reflections reflections) {
}

public static TypeHandlerLibrary forModuleEnvironment(ModuleManager moduleManager, TypeRegistry typeRegistry) {
TypeHandlerLibrary library = new TypeHandlerLibraryImpl(moduleManager, typeRegistry);

populateWithDefaultHandlers(library);

return library;
// The @Inject constructor already calls populateWithDefaultHandlers
return new TypeHandlerLibraryImpl(moduleManager, typeRegistry);
Copy link
Copy Markdown
Contributor

@BenjaminAmos BenjaminAmos Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will accept this as valid, however, the TODO comment did imply that this behaviour may have been intended to be temporary.

Copy link
Copy Markdown
Member

@Cervator Cervator Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair comment, and I suspect a bit of a tip of the ice berg when it comes to architectural quirks and tech debt built up over the years :-)

When we're through with these follow-up PRs I'd like to start writing a series of tests also meant to help solidify expectations and codifying them to make them easier to follow 👍

}

private static void populateWithDefaultHandlers(TypeHandlerLibrary serializationLibrary) {
Expand Down
4 changes: 2 additions & 2 deletions engine/src/main/java/org/terasology/engine/world/internal/EntityAwareWorldProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ public EntityAwareWorldProvider(WorldProviderCore base, Context context) {
}

@Inject
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());
}

Expand Down
13 changes: 9 additions & 4 deletions ...ain/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,15 @@ public Optional<T> deserialize(PersistedData data) {

if (fieldValue.isPresent()) {
if (Modifier.isPrivate(field.getModifiers())) {
try {
ReflectionUtil.findSetter(field).invoke(result);
} catch (InvocationTargetException e) {
logger.error("Failed to invoke setter for field {}", field);
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);
}
} else {
field.set(result, fieldValue.get());
Expand Down
Loading