Skip to content
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

Refactored 1.21 networking #29

Merged
merged 5 commits into from
Jul 28, 2024
Merged

Refactored 1.21 networking #29

merged 5 commits into from
Jul 28, 2024

Conversation

JR1811
Copy link

@JR1811 JR1811 commented Jul 14, 2024

Notes

  • moved a lot of packet related code into their respective packet records to avoid unnecessary static methods
    • packet records handle both sending and receiving now
    • only an instance of a BlockEntity on e.g. the client side is needed to send the actual packet this.blockEntitiy.createPacketData().send(); so block entities create the data for their packets. You can also just create a new instance of the packet manually if needed tho
  • separated payload registration with the registration of the receivers
  • PACKET_CODEC for enums uses short values now instead of int values since the amount of enum entries will never reach an integer size (even byte might be enough for most enums?)
  • removed using the ServerPlayConnectionEvents.INIT event and registered global receivers now
    • not sure if you needed the event for your specific use case, but normal global registration from the common entrypoint should be enough usually afaik.
  • removed now unnecessary thread checks when receiving packets

@JR1811
Copy link
Author

JR1811 commented Jul 14, 2024

This might conflict with #24 since that PR still used the old Networking system.
If needed this PR here can go after #24.

I should be able to fix the merge conflicts

@JR1811 JR1811 changed the title refactored 1.21 networking Refactored 1.21 networking Jul 15, 2024
private NetworkingUtil() {
}

public static boolean cantEditGlowcase(ServerPlayerEntity player, BlockPos pos, GlowcaseBlock glowcase) {
Copy link
Member

Choose a reason for hiding this comment

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

I know Intellij disagrees with me here, but I think it's more readable to have it be "canEditGlowcase" (even if that means it's always inverted)

this.url = tag.getString("url");
}

public EditHyperlinkBlock createPacketData() {
Copy link
Member

Choose a reason for hiding this comment

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

I might rename this to "createEditPacket" since I think that's more descriptive. But that'd be in a separate codestyle pass, nothing that would block this pr

Copy link
Author

@JR1811 JR1811 Jul 15, 2024

Choose a reason for hiding this comment

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

I guess this would apply for all the other BEs with packets too, then?

I wonder if something like an interface or abstract super class would be good to enforce / abstract this whole structure a bit better.

Copy link
Author

Choose a reason for hiding this comment

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

Also, I think I will move from short to byte for the enum packet codec entries too.

If there is ever going to be an enum with more entries than the byte size, there is something definitely wrong with the enum, and it should probably refactored to be something like a list or map...

Choose a reason for hiding this comment

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

my suggestion on this - EditPacket<T extends BlockEntity> extends CustomPayload

contains

  • void send() - defaulted c2s send.
  • void receive(context) - abstract for packets to implement
  • EditPacket create(T blockEntity) - construct packet from blockentity.

Anything that's not public from the BE that needs to be, just make it public.

@JR1811 JR1811 requested a review from TheEpicBlock July 24, 2024 00:00
# Conflicts:
#	src/main/java/dev/hephaestus/glowcase/block/entity/HyperlinkBlockEntity.java
#	src/main/java/dev/hephaestus/glowcase/block/entity/ItemDisplayBlockEntity.java
#	src/main/java/dev/hephaestus/glowcase/block/entity/TextBlockEntity.java
#	src/main/java/dev/hephaestus/glowcase/client/gui/screen/ingame/HyperlinkBlockEditScreen.java
#	src/main/java/dev/hephaestus/glowcase/client/gui/screen/ingame/ItemDisplayBlockEditScreen.java
#	src/main/java/dev/hephaestus/glowcase/client/gui/screen/ingame/TextBlockEditScreen.java
#	src/main/java/dev/hephaestus/glowcase/networking/GlowcaseClientNetworking.java
#	src/main/java/dev/hephaestus/glowcase/networking/GlowcaseCommonNetworking.java
this.url = tag.getString("url");
}

public EditHyperlinkBlock createPacketData() {

Choose a reason for hiding this comment

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

my suggestion on this - EditPacket<T extends BlockEntity> extends CustomPayload

contains

  • void send() - defaulted c2s send.
  • void receive(context) - abstract for packets to implement
  • EditPacket create(T blockEntity) - construct packet from blockentity.

Anything that's not public from the BE that needs to be, just make it public.

Copy link

@sisby-folk sisby-folk left a comment

Choose a reason for hiding this comment

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

Looking good! One main suggestion to better encapsulate packet logic.

@sisby-folk sisby-folk merged commit d4c8529 into ModFest:1.21 Jul 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants