Non-static API #125

MelonHell · 2024-01-28T23:01:59Z

I think this is a serious problem for Minestom and it needs to be addressed.

You cannot run two Minestom servers in one application.
It is impossible to replace components such as ConnectionManager with custom implementations.

When this is completed, it will definitely break backwards compatibility, but the sooner it is fixed, the less painful it will be

will resolves #70 and #119

mworzala · 2024-01-28T23:22:09Z

I'm probably down to merge this, but I will have to see of course

DasBabyPixel · 2024-01-29T07:39:10Z

Backwards compatibility could be kept if the static methods redirect to the first instance created. Kind of ugly, but that way there could be a deprecation phase to adapt. Not saying that this should be done, just pointing out options.

mworzala · 2024-01-29T13:17:17Z

I think it should probably just be done with another update like 1.20.5 or 1.21 which already may have breaking changes

MelonHell · 2024-01-29T16:26:11Z

Now it's running! I look forward to reviews, criticism and suggestions for improvement 🙂

mworzala · 2024-01-29T19:18:17Z

Small PR.... (I will start going through it soon)

wordandahalf · 2024-01-29T21:13:33Z

I'm skeptical of the need for MinecraftServerObject. It seems like an unneeded level of abstraction. In any case, I do think having a Server interface which MinecraftServer implements could provide flexibility useful to user.

MelonHell · 2024-01-30T07:35:24Z

In any case, I do think having a Server interface which MinecraftServer implements could provide flexibility useful to user.

I agree, I need to make an interface, otherwise this PR is useless :)

MelonHell · 2024-01-31T12:54:22Z

I thought it would be much simpler, but there is still a huge amount of work since many mistakes were made when designing Minestom

MelonHell · 2024-01-31T13:06:38Z

I think you can create a separate repository for this and call it minestom-ee 😄

mworzala · 2024-01-31T23:13:39Z

I just barely started reviewing, but in no world is Lombok being added to Minestom.

mworzala

I only got a small way through, will continue later.

demo/src/main/java/net/minestom/demo/Main.java

demo/src/main/java/net/minestom/demo/PlayerInit.java

mworzala · 2024-01-31T23:17:06Z

demo/src/main/java/net/minestom/demo/PlayerInit.java

@@ -187,24 +71,145 @@ public class PlayerInit {
        //     System.out.println("load end");
        // });

-        inventory = new Inventory(InventoryType.CHEST_1_ROW, Component.text("Test inventory"));
+        inventory = new Inventory(serverFacade.getGlobalEventHandler(), serverFacade.getServerSettings(), InventoryType.CHEST_1_ROW, Component.text("Test inventory"));


Inventory should probably get the server references it needs from the player it is opened for

help needed

mworzala · 2024-01-31T23:19:52Z

demo/src/main/java/net/minestom/demo/PlayerInit.java

+                    ItemStack droppedItem = event.getItemStack();
+
+                    Pos playerPos = player.getPosition();
+                    ItemEntity itemEntity = new ItemEntity(serverFacade, droppedItem);


There are a lot of odd cases created now because you could try to spawn an entity in a level of server A while passing server B into the constructor. These probably need to be handled somewhere and I'm not sure they are (im still on like file 2 of review)

In theory, two different servers can have common components, for example a common ExceptionHandler

mworzala · 2024-01-31T23:23:36Z

demo/src/main/java/net/minestom/demo/PlayerInit.java

-        }).repeat(10, TimeUnit.SERVER_TICK); //.schedule();
+            final Component footer = BenchmarkManager.getCpuMonitoringMessage(benchmarkManager);
+            serverFacade.getAudienceManager().players().sendPlayerListHeaderAndFooter(header, footer);
+        }).repeat(10, TimeUnit.getServerTick(serverFacade.getServerSettings())); //.schedule();


IMO another method should be added to take a SERVER_TICKS constant which the scheduler can then get from the server rather than it being a TimeUnit and requiring a reference to the server here.

SERVER_TICKS is no longer a constant, as it is a parameter that may differ between server instances

Yes I mean there should be a constant to represent server ticks and converted when encountered

help needed

src/main/java/net/minestom/server/ServerFacadeImpl.java

mworzala · 2024-01-31T23:29:17Z

src/main/java/net/minestom/server/ServerFacadeImpl.java

+        this.audienceManager = new AudienceManagerImpl(commandManager, this, this);
+        this.ticker = new TickerImpl(this);
+        this.serverStarter = new ServerStarter(this);
+        this.mojangAuth = new MojangAuth(this, this);


This should not be present unless used, perhaps it would be better to have some generic AuthProvider which this, velocity, bungee implement to prevent the confusion of enabling multiple at once.

I agree but I need help with this

src/main/java/net/minestom/server/ServerSettings.java

src/main/java/net/minestom/server/ServerStarter.java

mworzala · 2024-01-31T23:30:31Z

src/main/java/net/minestom/server/ServerStarter.java

+import java.util.concurrent.atomic.AtomicBoolean;
+
+@RequiredArgsConstructor
+public class ServerStarter {


I'm a little unclear on the need for this class

This class is needed to start the server (now renamed to ServerProcess). Since MinecraftServer is optional for running Minestom, there should be no logic in it, so the launch logic is in a separate class

MelonHell · 2024-02-01T07:32:49Z

build.gradle.kts

Tests will be fixed and enabled after other changes are approved

"The server from the Instance should be passed into the chunk supplier, this should remain a method reference."

MelonHell · 2024-02-07T22:58:37Z

src/main/java/net/minestom/server/Viewable.java

@@ -58,40 +58,42 @@ default boolean isViewer(@NotNull Player player) {
     *
     * @param packet the packet to send to all viewers
     */
-    default void sendPacketToViewers(@NotNull SendablePacket packet) {
+    default void sendPacketToViewers(ServerSettingsProvider serverSettingsProvider, @NotNull SendablePacket packet) {


I don't like that ServerSettings is required here, but I don't know how to fix it

MelonHell · 2024-02-07T23:05:20Z

I need help with:

Who is interested in this pull request please help me 🥺

Rollczi · 2024-02-10T22:04:39Z

Make Inventory undepended from GlobalEventHandler and ServerSettings

@MelonHell Hi, I think you can move some dependencies:
from

we can do this by using interfaces to dependency inversion:
Player can implement EventReceiver (I don't know how to name it exactly), also we can add a interface for PlayerInventory.

Generally, this is a solution to this problem, but I also have a proposal to create a class "InventoryFactory"/"InventoryManager", which will create new inventory. This may help create new inventories that will need additional dependencies.

MelonHell added 2 commits January 29, 2024 01:53

started rewriting

d07a151

more

1c2b172

and more

43a2f6a

MelonHell added 3 commits January 29, 2024 17:14

Moooooore

8e17990

and more...

c6e3815

it starts!

bef47b8

MelonHell marked this pull request as ready for review January 29, 2024 16:27

mworzala linked an issue Jan 29, 2024 that may be closed by this pull request

Add more Object-oriented programming API. #119

Open

MelonHell and others added 3 commits January 30, 2024 15:26

now it looks good

2f49b70

little changes

5ad4b14

Merge branch 'main' into patch/oop-api

07ad6fd

MelonHell force-pushed the patch/oop-api branch from 9853e10 to 07ad6fd Compare January 30, 2024 13:58

kill me plz

a8ef545

MelonHell added 2 commits January 31, 2024 19:45

little improvements

253970b

more little improvements

a700e6b

mworzala suggested changes Jan 31, 2024

View reviewed changes

MelonHell added 3 commits February 1, 2024 10:08

renamed ServerFacade to MinecraftServer

d5fd344

Delombok :(

0f31481

rename ServerStarter to ServerProcess

7273176

MelonHell commented Feb 1, 2024

View reviewed changes

build.gradle.kts Outdated

Copy link

Author

MelonHell Feb 1, 2024

Choose a reason for hiding this comment

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

Tests will be fixed and enabled after other changes are approved

MelonHell added 2 commits February 1, 2024 10:45

fix getters after delombok (Player)

ea58151

simplify chunk constructor

dce8a8f

"The server from the Instance should be passed into the chunk supplier, this should remain a method reference."

MelonHell commented Feb 7, 2024

View reviewed changes

Rollczi mentioned this pull request Sep 16, 2024

Add more Object-oriented programming API. Minestom/Minestom#2404

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-static API #125

Non-static API #125

MelonHell commented Jan 28, 2024

mworzala commented Jan 28, 2024

DasBabyPixel commented Jan 29, 2024

mworzala commented Jan 29, 2024

MelonHell commented Jan 29, 2024

mworzala commented Jan 29, 2024

wordandahalf commented Jan 29, 2024

MelonHell commented Jan 30, 2024 •

edited

Loading

MelonHell commented Jan 31, 2024

MelonHell commented Jan 31, 2024

mworzala commented Jan 31, 2024

mworzala left a comment

mworzala Jan 31, 2024

MelonHell Feb 7, 2024

mworzala Jan 31, 2024

MelonHell Feb 1, 2024

mworzala Jan 31, 2024

MelonHell Feb 1, 2024 •

edited

Loading

mworzala Feb 1, 2024

MelonHell Feb 7, 2024

mworzala Jan 31, 2024

MelonHell Feb 1, 2024

mworzala Jan 31, 2024

MelonHell Feb 1, 2024

MelonHell Feb 1, 2024

MelonHell Feb 7, 2024

MelonHell commented Feb 7, 2024

Rollczi commented Feb 10, 2024

Non-static API #125

Are you sure you want to change the base?

Non-static API #125

Conversation

MelonHell commented Jan 28, 2024

mworzala commented Jan 28, 2024

DasBabyPixel commented Jan 29, 2024

mworzala commented Jan 29, 2024

MelonHell commented Jan 29, 2024

mworzala commented Jan 29, 2024

wordandahalf commented Jan 29, 2024

MelonHell commented Jan 30, 2024 • edited Loading

MelonHell commented Jan 31, 2024

MelonHell commented Jan 31, 2024

mworzala commented Jan 31, 2024

mworzala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelonHell Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelonHell commented Feb 7, 2024

Rollczi commented Feb 10, 2024

MelonHell commented Jan 30, 2024 •

edited

Loading

MelonHell Feb 1, 2024 •

edited

Loading