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 4 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
3 changes: 2 additions & 1 deletion ...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,8 @@
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;

Check warning on line 12 in engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRemoteWorld.java

View check run for this annotation

Terasology Jenkins.io / CheckStyle

UnusedImportsCheck 

NORMAL:
Unused import - org.terasology.engine.entitySystem.entity.EntityManager.
Raw output 
<p>Since Checkstyle 3.0</p><p> Checks for unused import statements. Checkstyle uses a simple but very reliable algorithm to report on unused import statements. An import statement is considered unused if: </p><ul><li> It is not referenced in the file. The algorithm does not support wild-card imports like <code>import java.io.*;</code>. Most IDE's provide very sophisticated checks for imports that handle wild-card imports. </li><li> It is a duplicate of another import. This is when a class is imported more than once. </li><li> The class imported is from the <code>java.lang</code> package. For example importing <code>java.lang.String</code>. </li><li> The class imported is from the same package. </li><li><b>Optionally:</b> it is referenced in Javadoc comments. This check is on by default, but it is considered bad practice to introduce a compile time dependency for documentation purposes only. As an example, the import <code>java.util.Date</code> would be considered referenced with the Javadoc comment <code>{@link Date}</code>. The alternative to avoid introducing a compile time dependency would be to write the Javadoc comment as <code>{@link java.util.Date}</code>. </li></ul><p> The main limitation of this check is handling the case where an imported type has the same name as a declaration, such as a member variable. </p><p> For example, in the following case the import <code>java.awt.Component</code> will not be flagged as unused: </p><pre><code> import java.awt.Component; class FooBar { private Object Component; // a bad practice in my opinion ... } </code></pre>
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 +92,7 @@
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
3 changes: 2 additions & 1 deletion 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,8 @@
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;

Check warning on line 18 in engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseWorld.java

View check run for this annotation

Terasology Jenkins.io / CheckStyle

UnusedImportsCheck 

NORMAL:
Unused import - org.terasology.engine.entitySystem.entity.EntityManager.
Raw output 
<p>Since Checkstyle 3.0</p><p> Checks for unused import statements. Checkstyle uses a simple but very reliable algorithm to report on unused import statements. An import statement is considered unused if: </p><ul><li> It is not referenced in the file. The algorithm does not support wild-card imports like <code>import java.io.*;</code>. Most IDE's provide very sophisticated checks for imports that handle wild-card imports. </li><li> It is a duplicate of another import. This is when a class is imported more than once. </li><li> The class imported is from the <code>java.lang</code> package. For example importing <code>java.lang.String</code>. </li><li> The class imported is from the same package. </li><li><b>Optionally:</b> it is referenced in Javadoc comments. This check is on by default, but it is considered bad practice to introduce a compile time dependency for documentation purposes only. As an example, the import <code>java.util.Date</code> would be considered referenced with the Javadoc comment <code>{@link Date}</code>. The alternative to avoid introducing a compile time dependency would be to write the Javadoc comment as <code>{@link java.util.Date}</code>. </li></ul><p> The main limitation of this check is handling the case where an imported type has the same name as a declaration, such as a member variable. </p><p> For example, in the following case the import <code>java.awt.Component</code> will not be flagged as unused: </p><pre><code> import java.awt.Component; class FooBar { private Object Component; // a bad practice in my opinion ... } </code></pre>
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 +161,7 @@
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.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
*/
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.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
*/
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);

@BenjaminAmos BenjaminAmos Apr 2, 2026

Copy link
Copy Markdown
Contributor

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.

@Cervator Cervator Apr 2, 2026

Copy link
Copy Markdown
Member

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