-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
chore: CoreRegistry removal from engine.network #5033
Conversation
97a6b0e
to
6b569ae
Compare
After asking #gestalt and seeing some constructor injector usage with the upcoming gestalt-di, I went with that approach. It felt safer than field injection, as I know I can't create an instance without filling those values this way. |
Fixes some race conditions in MTE tests. test(network): ClientHandler's GameEngine reference is optional? seems sus, but it's only used for a disconnection event, so maybe.
6b569ae
to
da898bc
Compare
got some problems using this as a server; switching to Draft while I figure that out |
with some documentation
23c186e
to
6647a4e
Compare
okay, that was one part boring NPE and one part not knowing how to run a server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't test it yet, but code looks good to me 👍
Left a few questions, but none of them are really blocking.
@@ -110,6 +111,7 @@ public void init(GameEngine engine) { | |||
headless = context.get(DisplayDevice.class).isHeadless(); | |||
|
|||
CoreRegistry.setContext(context); | |||
context.getValue(NetworkSystem.class).setContext(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? We are getting the network system out of the context, and then set the context for that network system?
Does it create a "back link" from the network system to the context it lives in? Or is the network system distributing the context further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See
Terasology/engine/src/main/java/org/terasology/engine/network/internal/NetworkSystemImpl.java
Lines 778 to 786 in 6647a4e
/** | |
* Sets the context within which this system should operate. | |
* <p> | |
* As a client transitions from connecting to loading to in-game, it moves | |
* through different {@link org.terasology.engine.core.modes.GameState GameStates}, | |
* and the context changes along the way. | |
*/ | |
@Override | |
public void setContext(Context newContext) { |
I'm not 100% certain this addition is necessary, as I put it in during some troubleshooting and didn't confirm later. But it's near where the context is created, and it's being set in CoreRegistry which NetworkSystem was using, so it's probably a good idea.
@@ -23,7 +24,7 @@ public String getName() { | |||
|
|||
@Override | |||
public void initialise(GameEngine engine, Context rootContext) { | |||
networkSystem = new NetworkSystemImpl(rootContext.get(Time.class), rootContext); | |||
networkSystem = new NetworkSystemImpl((EngineTime) rootContext.get(Time.class), rootContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ... unsafe 🤔 If it is important that it is an EngineTime
, why don't we put it in the context as EngineTime
instead of Time
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. I wondered that too.
My best guess is that Time
is the read-only interface, and the one that gets put in Context for most common use, while EngineTime
has other setter methods on it that the network system needs to synchronize time between client and server.
But as we can see, "please don't cast this" is not an effective form of capability control, so 🤷
public ClientHandler(NetworkSystemImpl networkSystem) { | ||
@SuppressWarnings("OptionalUsedAsFieldOrParameterType") | ||
public ClientHandler(NetworkSystemImpl networkSystem, Optional<GameEngine> gameEngine) { | ||
this.gameEngine = gameEngine.orElse(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this behave if the engine actually happens to be `null? Will we get a reasonable error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its only usage is here:
Terasology/engine/src/main/java/org/terasology/engine/network/internal/ClientHandler.java
Lines 37 to 39 in 6647a4e
public void channelInactive(ChannelHandlerContext ctx) throws Exception { | |
if (gameEngine != null) { | |
gameEngine.changeState(new StateMainMenu("Disconnected From Server")); |
I still think it's a little suspect, but it's the nearest replacement to how CoreRegistry.get
works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll have to ask SpotBugs to chill with the EI_EXPOSE_REP2
@@ -23,7 +24,7 @@ public String getName() { | |||
|
|||
@Override | |||
public void initialise(GameEngine engine, Context rootContext) { | |||
networkSystem = new NetworkSystemImpl(rootContext.get(Time.class), rootContext); | |||
networkSystem = new NetworkSystemImpl((EngineTime) rootContext.get(Time.class), rootContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. I wondered that too.
My best guess is that Time
is the read-only interface, and the one that gets put in Context for most common use, while EngineTime
has other setter methods on it that the network system needs to synchronize time between client and server.
But as we can see, "please don't cast this" is not an effective form of capability control, so 🤷
public ClientHandler(NetworkSystemImpl networkSystem) { | ||
@SuppressWarnings("OptionalUsedAsFieldOrParameterType") | ||
public ClientHandler(NetworkSystemImpl networkSystem, Optional<GameEngine> gameEngine) { | ||
this.gameEngine = gameEngine.orElse(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its only usage is here:
Terasology/engine/src/main/java/org/terasology/engine/network/internal/ClientHandler.java
Lines 37 to 39 in 6647a4e
public void channelInactive(ChannelHandlerContext ctx) throws Exception { | |
if (gameEngine != null) { | |
gameEngine.changeState(new StateMainMenu("Disconnected From Server")); |
I still think it's a little suspect, but it's the nearest replacement to how CoreRegistry.get
works.
…egistry # Conflicts: # engine/src/main/java/org/terasology/engine/context/internal/ContextImpl.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in local multiplay with a couple of bugs (which are also present on develop
). Could connect just fine, so LGTM.
Fixes some race conditions in MTE tests.
Fixes #5030.
How to test
Test all the networking things!
Join server, leave server, browse server list, etc.