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

Initial 16-bit Metadata Re-work #7

Merged
merged 9 commits into from
Feb 9, 2024
Merged

Initial 16-bit Metadata Re-work #7

merged 9 commits into from
Feb 9, 2024

Conversation

Cleptomania
Copy link
Member

This PR increases the metadata value from 4 bits to 16 bits.

Most calculations in this are explained in comments above the magic numbers and most are using constants now.

If the postNeidWorldsSupport config option is enabled, then it will function similarly to how that is handled for block IDs, metadatas which are outside of the 0-15 range supported by vanilla will be saved as a 0, and anything within 0-15 will be saved accurately as it's normal vanilla flavor. This means that if you remove NEID from the pack, the world isn't necessarily fully corrupted. A world loaded without NEID that triggers this should not break the NEID data, as the NEID metadata is stored in a different NBT tag(Data16) vs the vanilla Data tag. This is the same way this option functions for block IDs.

@Dream-Master Dream-Master requested a review from a team December 18, 2023 20:39
@Dream-Master Dream-Master marked this pull request as draft December 19, 2023 10:56
@Dream-Master
Copy link
Member

@Cleptomania you named it draft so i set it draft. undraft it when it can be reviewed.

@Cleptomania
Copy link
Member Author

I think this is ready for review and to start considering merging it.

Putting the metadata changes behind a config option was somewhat discussed, but I would rather not do that. Partially because it makes the code much more complex(the block ID extension and metadata extension touch basically all the same things, doing it differently conditionally will be annoying).

The other big problem is that the metadata extension only really makes sense in the context that mods are utilizing it, it would make the chain of this being a dependency of another mod for the metadata extension trickier if that depends on a config option(we could make it the default, but still).

My proposal would be that the metadata extension gets merged and released as either version 2.0 or 1.6, this leaves us the following versioning profile:
Pre 1.5: Original ASM version, pre mixin re-write
1.5.X: Same functionality as ASM version, but with mixins and an interface for interop with ExtendedBlockStorage
2.X: Metadata extension

Once we merge this and release that version(be it 1.6 or 2.0) we could do an initial test of it in nightlies, so far the only bugs I've been able to produce are when mods make use of the extended metadata and don't adequately update themselves to be aware of that, but that is more a problem of the specific mods which leverage it than NEID itself.

@Cleptomania Cleptomania marked this pull request as ready for review January 12, 2024 21:41
@Cleptomania Cleptomania changed the title DRAFT: Initial 16-bit Metadata Re-work Initial 16-bit Metadata Re-work Jan 12, 2024
@Cleptomania
Copy link
Member Author

This now additionally adds GTNHLib as a dependency(0.2.1+, for fastutil shadow), and also converts the config to use GTNHLib(old config file is compatible and will work fine).

Copy link
Contributor

@Connor-Colenso Connor-Colenso left a comment

Choose a reason for hiding this comment

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

I've been using this without issue for well over a month now in dev and it loads fine. Code looks fine and it's already in nightly.

@Connor-Colenso Connor-Colenso merged commit b6221e4 into GTNewHorizons:master Feb 9, 2024
1 check passed
@chochem
Copy link
Member

chochem commented Feb 9, 2024

it's already in nightly.

the nightly that is so very broken? I mean its probably not this but it being in nightly is in argument against right now, not in favor ;)

@Pilzinsel64
Copy link

Pilzinsel64 commented Feb 10, 2024

I just tried v2.0.0 on my test server (copy of my prod instance, not GTNH, but using a lot GTNH mods). Each single time I try to join I my client crashes with an IndexOutOfBoundsException with no more useful information. As soon as I go back to 1.5.6 I can join the server again. Not sure if this is my somehow my fault or depent on this PR. 🤔

java.lang.ArrayIndexOutOfBoundsException
	at java.lang.System.arraycopy(Native Method)
	at net.minecraft.world.chunk.Chunk.func_76607_a(Chunk.java:1209)
	at net.minecraft.client.network.NetHandlerPlayClient.func_147269_a(NetHandlerPlayClient.java:1194)
	at net.minecraft.network.play.server.S26PacketMapChunkBulk.func_148833_a(S26PacketMapChunkBulk.java:184)
	at net.minecraft.network.play.server.S26PacketMapChunkBulk.func_148833_a(S26PacketMapChunkBulk.java:242)
	at net.minecraft.network.NetworkManager.func_74428_b(NetworkManager.java:212)
	at net.minecraft.client.multiplayer.PlayerControllerMP.func_78765_e(PlayerControllerMP.java:273)
	at net.minecraft.client.Minecraft.func_71407_l(Minecraft.java:1602)
	at net.minecraft.client.Minecraft.func_71411_J(Minecraft.java:973)
	at net.minecraft.client.Minecraft.func_99999_d(Minecraft.java:898)
	at net.minecraft.client.main.Main.main(SourceFile:148)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at net.minecraft.launchwrapper.Launch.launch(Launch.java:135)
	at net.minecraft.launchwrapper.Launch.main(Launch.java:28)
	at org.prismlauncher.launcher.impl.StandardLauncher.launch(StandardLauncher.java:87)
	at org.prismlauncher.EntryPoint.listen(EntryPoint.java:130)
	at org.prismlauncher.EntryPoint.main(EntryPoint.java:70)

Full log:
fml-client-1.log

@Cleptomania
Copy link
Member Author

@Pilzinsel64 do you have any config that differs from GTNH for ArchaicFix? I'm unable to get this to re-produce with GTNH nightly 341(using NEID 2.0.0). I also tested with Optifine added.

I'm not actually sure what in ArchaicFix could break it, I just see a lot of mixins from it in the stacktrace, so potentially something there.

@Pilzinsel64
Copy link

Pilzinsel64 commented Feb 10, 2024

do you have any config that differs from GTNH for ArchaicFix?

Of course I have a lot of configs different probably. However, I can't find a config for ArchaicFix in the GTNH modpack.

But good to know that it works on your side, so it might be my fault somewhere.

I checked again with ArchaicFix disabled and it works! I then enabled it and turned off optimizeObjectIntIdentityMap and it works. Then I enabled optimizeObjectIntIdentityMap back on and it still works. So, I have no idea why it now works. It also happens without ArchaicFix.

I also tested EndlessIDs with 32bits configured but there I directly get errors from ExtraBiomesXL. Need to test a little bit more around and reset some configs (looks lik EBXL generates itself the config dependend on what is possible). But that's another story.

Will do some tests against tomorrow but yea, that's absolutely on my side. Thanks anyway and sorry for bothering. :)

@Cleptomania
Copy link
Member Author

If you do happen to find where the problem is, let me know what you find and I can at least try and add compat for whatever that scenario/combination of mods/config is.

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.

6 participants