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

chore: port Aether to 1.20.4 #2073

Merged

Conversation

GizmoTheMoonPig
Copy link
Contributor

This PR relies on my Nitrogen port to function. Let me know when its merged and i'll update the buildscript accordingly.

Changes in this port:

  • altar, freezer, and incubator fuels have been moved to their own data maps. This unhardcodes the current fuels and removes the need for CraftTweaker to change them
  • moved all capabilities to the new attachment system
  • refactored packets to use the new network system
  • removed all uses of FinishedRecipe and instead used the recipe itself when serializing as FinishedRecipe is gone
  • fixed an issue where the altar repairing recipes used the wrong serializer
  • Changed blocks and items to use DeferredBlock and DeferredItem as both classes implement Holder, Supplier, and Itemlike, allowing for easier use in many scenarios.
  • lots of miscellaneous other changes to account for
  • ran a code cleanup to fix inconsistent spacing in the codebase, remove unused imports, and clean up javadocs. I also added an editorconfig so contributor's IDEs are automatically configured to use the project settings. I chose to use 4 spaces as the indent as like 90% of the codebase was already using that. Please let me know if this isnt desired and i'll remove it.

I agree to the Contributor License Agreement (CLA).

@bconlon1 bconlon1 self-requested a review March 22, 2024 04:07
@bconlon1 bconlon1 added priority/high This issue is of high priority to fix. status/pending-review The issue references a pull request which is pending review. version/1.20.4 This issue affects Minecraft version 1.20.4. labels Mar 22, 2024
@Jaryt
Copy link
Contributor

Jaryt commented Mar 24, 2024

Hi,

Super cool to see this contribution :D Need to say tho, would have been nice if this was chunked up a little bit. I get that ports can be a bit annoying where you have to fix everything at once to get it running, but having a bunch of json 1 liners and style formatting changes mixed in with actual updates makes this PR super hard to review properly.

Not dismissing the hard work here, but ideally each one of those points you've made would have been a single PR in of itself. That helps the reviewer know what changes they're looking for, rather than just glossing over a lot of things and possibly hitting you with a flat out rejection.

If there is anyway you could break this out into multiple PRs, that would be very helpful to the reviewer.

Thanks :)

@bconlon1
Copy link
Contributor

Hi,

Super cool to see this contribution :D Need to say tho, would have been nice if this was chunked up a little bit. I get that ports can be a bit annoying where you have to fix everything at once to get it running, but having a bunch of json 1 liners and style formatting changes mixed in with actual updates makes this PR super hard to review properly.

Not dismissing the hard work here, but ideally each one of those points you've made would have been a single PR in of itself. That helps the reviewer know what changes they're looking for, rather than just glossing over a lot of things and possibly hitting you with a flat out rejection.

If there is anyway you could break this out into multiple PRs, that would be very helpful to the reviewer.

Thanks :)

I've been working on reviewing this and have gotten through the JSON changes but I haven't had time to finish yet. In terms of reviewability it doesn't bother me personally, even if long lists of code changes start to make GitHub struggle. On that note though, if there's easy ways to split things up that could be useful to teach to make things easier to view, but I do not mind the PR format, and I think trying to split something like a port can end up having more complications than making things easier.

@GizmoTheMoonPig
Copy link
Contributor Author

Yeah I realize it probably would've been good to split this up into separate commits. Whenever I port projects I normally do it all in 1 go so I was just doing what I'm used to. I'll definitely try to split it up more if I ever end up porting the project again

@Firestar99
Copy link

Using this branch a client connecting to a dedicated server will immediately disconnect due to a message writing to a ByteBuf throwing an exception. Did not yet debug it further. (yes I couldn't wait for the update and gone and compiled it myself, knowing the risk that things may break 😄 )

io.netty.handler.codec.EncoderException: java.lang.ClassCastException: class java.util.Optional cannot be cast to class java.util.UUID (java.util.Optional and java.util.UUID are in module java.base of loader 'bootstrap')
	at io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:107) ~[netty-codec-4.1.97.Final.jar%2396!/:4.1.97.Final] {}
	at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:881) ~[netty-transport-4.1.97.Final.jar%23135!/:4.1.97.Final] {}
	at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:863) ~[netty-transport-4.1.97.Final.jar%23135!/:4.1.97.Final] {}
	at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:968) ~[netty-transport-4.1.97.Final.jar%23135!/:4.1.97.Final] {}
	at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:856) ~[netty-transport-4.1.97.Final.jar%23135!/:4.1.97.Final] {}
	at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:851) ~[netty-transport-4.1.97.Final.jar%23135!/:4.1.97.Final] {}
	at io.netty.channel.DefaultChannelPipeline.write(DefaultChannelPipeline.java:1010) ~[netty-transport-4.1.97.Final.jar%23135!/:4.1.97.Final] {}
	at io.netty.channel.AbstractChannel.write(AbstractChannel.java:296) ~[netty-transport-4.1.97.Final.jar%23135!/:4.1.97.Final] {}
	at net.minecraft.network.Connection.doSendPacket(Connection.java:310) ~[neoforge-20.4.200.jar%23183!/:?] {re:classloading}
	at net.minecraft.network.Connection.lambda$sendPacket$9(Connection.java:305) ~[neoforge-20.4.200.jar%23183!/:?] {re:classloading}
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174) ~[netty-common-4.1.97.Final.jar%2397!/:4.1.97.Final] {}
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167) ~[netty-common-4.1.97.Final.jar%2397!/:4.1.97.Final] {}
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470) ~[netty-common-4.1.97.Final.jar%2397!/:4.1.97.Final] {}
	at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:413) ~[netty-transport-classes-epoll-4.1.97.Final.jar%23121!/:4.1.97.Final] {}
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[netty-common-4.1.97.Final.jar%2397!/:4.1.97.Final] {}
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.97.Final.jar%2397!/:4.1.97.Final] {}
	at java.lang.Thread.run(Thread.java:833) ~[?:?] {}
Caused by: java.lang.ClassCastException: class java.util.Optional cannot be cast to class java.util.UUID (java.util.Optional and java.util.UUID are in module java.base of loader 'bootstrap')
	at com.aetherteam.nitrogen.network.packet.SyncPacket.write(SyncPacket.java:39) ~[nitrogen_internals-1.20.4-1.0.23-neoforge.jar%23186!/:1.20.4-1.0.23-neoforge] {re:classloading}
	at com.aetherteam.nitrogen.network.packet.SyncEntityPacket.write(SyncEntityPacket.java:32) ~[nitrogen_internals-1.20.4-1.0.23-neoforge.jar%23186!/:1.20.4-1.0.23-neoforge] {re:classloading}
	at net.minecraft.network.protocol.common.ClientboundCustomPayloadPacket.write(ClientboundCustomPayloadPacket.java:117) ~[neoforge-20.4.200.jar%23183!/:?] {re:classloading,pl:accesstransformer:B}
	at net.neoforged.neoforge.network.filters.GenericPacketSplitter.encode(GenericPacketSplitter.java:107) ~[neoforge-20.4.200.jar%23182%23185!/:?] {re:classloading}
	at net.neoforged.neoforge.network.filters.GenericPacketSplitter.encode(GenericPacketSplitter.java:45) ~[neoforge-20.4.200.jar%23182%23185!/:?] {re:classloading}
	at io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:90) ~[netty-codec-4.1.97.Final.jar%2396!/:4.1.97.Final] {}
	... 16 more

@GizmoTheMoonPig
Copy link
Contributor Author

should be fixed in the newest commit. Bit of an oversight, my bad

Copy link
Contributor

@bconlon1 bconlon1 left a comment

Choose a reason for hiding this comment

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

lgtm

@bconlon1 bconlon1 merged commit ff1c0f8 into The-Aether-Team:1.20.4-develop Apr 1, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high This issue is of high priority to fix. status/pending-review The issue references a pull request which is pending review. version/1.20.4 This issue affects Minecraft version 1.20.4.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants