Fix Granite ID Shift

[serenibyss]

Introduction

First I would like to preface this by saying that I feel this PR is Priority 1 as a candidate for next release.

Let me try to explain this in a concise and coherent way, as best I can.

This PR fixes the ID shift introduced by Granite being added as a GregTech block, and ensures that it will never happen again. It does so by fixing the issue in 2 parts:

• Rework the compressed block MetaBlock allocation by ensuring there can be no overlap, by setting its ID to materialID / 16 and its meta ID to materialID % 16. As stated in a comment on the original issue post,

  For example HSS-G has the material ID 302. You would perform integer division and modulus with 16 (the cap for MetaBlock subitems) resulting in 302 / 16 = 18 and 302 % 16 = 14. Thus the item "Block of HSS-G" would be: (with the current scheme): gregtech:compressed_18:14.

• Remap MetaBlocks (and SurfaceRocks, since they could also be affected similarly) to an entirely new naming convention.

  • OLD: gregtech:compressed_xx:xx, NEW: gregtech:meta_block_compressed_xx:xx
  • OLD: gregtech:surface_rock_x,    NEW: gregtech:meta_block_surface_rock_x

  Doing this ensures two things:

  • Scripts using something like <gregtech:compressed_10:10> will very clearly break, instead of being impacted by the remap in invisible ways, leading to recipes with unpredictable ingredients.
  • Updates all MetaBlocks and SurfaceRocks to the new mapping scheme in a very visible way, but without breaking any existing blocks in the world, in inventories, on AE patterns, etc..

DataFixer Implementation

To start, we declare a few states our Data Fixers can be in:

• -1: Represents a world before Granite was given a Block.
• 0: Represents after the Granite block was added.
• 1: Represents our updated state, with remapping already applied. No fixers will be run in this state currently.

We can add onto these states if the time comes for another ID shift, simply by adding a new state here and handling its cases in the DataFixers.

Next, we declare our data fixers to fix the following categories:

• BLOCK_ENTITY: Runs for any Tile Entity encountered.
• ENTITY: Runs for any Entity encountered, like ItemStacks on the ground for example.
• PLAYER: Runs for player inventories when joining a server.
• CHUNK: Runs for the entire NBTCompound of a chunk.

As well as some custom defined types:

• ITEM_STACK_LIKE: Runs for anything appearing to be an ItemStack, with a tag {String id, byte count, short damage}.
• CHUNK_SECTION: Runs for a vertical section of a chunk containing BlockState data.

Then we patch in some method calls into Forge code.

• First into CompoundDataFixer#getVersion, so that we can accurately discern the version of our fixers to use.
• Next into SafeFormatOld#loadAndFix, so that we can properly write and read data from the level.dat to know if we need to run our fixers.

Then, once we have tested for the world GregTech version, if we need to run fixers, we first call a StartupQuery similar to what Forge does when it detects missing registry IDs from the world. We consume and ignore those calls, so the default Forge query will not appear and ours will instead. First, we display a warning and ask if the player wants to create a backup before proceeding, where they can opt-out without cancelling the world load:

(Note here that we are using the version from the major ID state discussed earlier, and not the exact GTCE version of the world they are loading)

If they choose to opt-out of this backup, we display one more warning, ensuring they meant to do this. If they decide to cancel here, the world load will be aborted.

Then, if the player proceeds with the world load, the DataWalkers are called and begin remapping the NBT data. These walkers cover all cases, including but not limited to:

• Placed blocks in world
• Vanilla Chests, GT Chests, Storage Drawers, and other ItemStack containers
• Player Inventory
• AE2/RS Storage Cells, Patterns, Export Bus data, EIO Conduit filters, etc.

At this point, the world's level.dat file now contains data on it showing that the remapping has taken place, so it does not need to happen again. This ensures that the backup prompt does not occur more than once for any given world.

Test Results

• I created a fresh world on 1.13.0 with nothing added. Then I tested it with the fixers, and saw no issues.
• I created a fresh world on 1.13.0 with some compressed blocks placed, and put some into some GT and Vanilla chests and Quantum Chests. Then I tested it with the fixers, and saw no issues.
• I created a fresh world on 1.13.0 with Gregicality, and one with SoG, and performed similar steps to the one above. Then I tested it with the fixers, and saw no issues.
• I created a fresh world on Omnifactory latest dev instance (using GTCE 1.12.1), and added some compressed blocks and put some in storage containers, as well as some other random GT machines and things from other mods. I then tested it by replacing the GTCE jar with the one with fixers, and saw no issues.
• I created an instance Omnifactory latest dev (using GTCE 1.12.1), and imported a large late-game world, filled with complex NBT like Storage Drawers, AE2 patterns, etc.. Then I tested it with the fixers jar, and saw no issues. AE2 patterns with compressed blocks in them had full functionality, drawers had their contents properly, etc..
• I created an instance of Omnifactory latest dev (using GTCE 1.12.1) and imported a large late-game world from Omnifactory 1.2.2 (using GTCE 1.8.4.419). I did an initial launch of this world in the updated Omnifactory instance using the fixers jar, simulating if a player of the modpack received an update including the Data Fixers. I saw no issues, similar to the test case above.

With these test cases, I am confident that players will not have any impact on their GTCE experience, and that all blocks will be properly mapped, completely skipping versions of GTCE where there were ID shift issues. Additionally, addons who use our material system extensively (like Gregicality) will have no issues with their materials that generate compressed blocks.

Merging Notes

There is a field in WorldDataHooks currently named V1_14_0. This field needs to be named and set to whatever version of GTCE these commits are included in, as it signifies which version our new meta block remapping scheme began in. I named it 1.14.0 since I anticipate that to be the version this is included in, but if that turns out to be different, it will need to be adjusted accordingly.

Compatibility

This PR remaps all compressed blocks from compressed_xx to meta_block_compressed_xx. This is obviously a breaking change for scripts. However, it is a very clear change, as the registry name is fully distinct from what it was previously, so there will not be "invisible" changes to IDs, and scripts will simply error instead of accidentally using the wrong block in a recipe. Additionally, this incentivizes modpack authors to use the new MetaItem bracket handler upgrade implemented in #1352.

Credits

• @phantamanta44 For implementing the data fixers
• @Exaxxion For implementing and creating the new block mappings, and JUnit testing
• @serenibyss For implementing the backup system and the level.dat version scraping
• @ALongStringOfNumbers For moral support and help with brainstorming :)

And lastly,

• Closes [BUG] Metadata shift of some blocks after adding Block of Granite #1361

[LAGIdiot]

Thank you for taking over such hard task and moving it forward. This is definitely big piece of work.

Opening comment is incredible (except that gif...). It is easy to understand and it covers all fundamental points.

I have tried this on my dev setup but it seems that I have some environment issues as it did not trigger onWorldLoad but fixes were applied correctly. Also my log is full of standard FML missing id and wrong id found. I will have to try this on clean setup.

I have finish code review and I have commented on some places that I feel need adjustments. As always I am opened to discussion on these points as my vision may not be correct.

[serenibyss]

Force push was to rebase to fix a merge conflict

[serenibyss]

Also my log is full of standard FML missing id and wrong id found

We consume the warning/error calls that Forge puts into the log to avoid the Forge StartupQuery, but I do not know if there is any way to get the standard info logging calls to go away

[serenibyss]

Issue found:
I did these steps:

1. Create world pre-fixers
2. Go to x1000, z1000 and place compressed blocks
3. Go to x0, z0
4. Close world and launch with fixers
5. Load world, observe my inventory was fixed
6. Save and close world, close game
7. Relaunch with fixers, go to x1000, z1000

Placed blocks are gone. We will have to change this to run the fixers on a world created before GTCE v1.14.0 every time the world is launched. Additionally, we will have to make sure that the backup prompt is only called on the first launch of the world with the fixers.

[LAGIdiot]

So fixers are not one time thing which will load everything and fix it. But they have to be active every lunch and will be fixing it as it loads correct?

[serenibyss]

My understanding is that they will always be active on every version after this is released, but the issue I described happens because it loses the mapping from old->new. We need to save that mapping somewhere. Phanta is working on it I believe.

[LAGIdiot]

I see that there were some updates. Is this ready for another review (all my concerns regarding code and DStrand1's regarding functionality were fixed/answered) or is it still work in progress?

[phantamanta44]

theoretically, everything should work, but we haven't had the chance to test extensively yet

[LAGIdiot]

As this was quite a rewrite additional testing is indeed needed to one that was surely done when opening this PR. We definitely can't rush this as impact of this is really huge. And for that I will not include it in next release (which means this will need update of versions inside, but that is small change).

From quick look I give this I feel that WorldDataHooks is there twice but only one is used and createGeneratedBlockDeprecated is used only in tests. So I expect that you are planning some cleanup after testing? Or should I do full code review on this and then there will be clean up and testing?

[phantamanta44]

yeah, was gonna clean up later after ensuring that everything is functionally correct

[serenibyss]

The force push was to rebase with the new MCP mapping, and to fix a merge conflict with the asm transformer.

The Omnifactory team has opened up public testing for a version of their pack including a prerelease jar with these datafixers in them. So, hopefully we can find any issues with it during that testing, or get some reassurance that it is ready to go.

See Nomifactory/Nomifactory#726 for the test results we have collected from the community.

[phantamanta44]

for posterity: the fix fails to deal with the ender chests from the Ender Storage mod. it seems to load its chest data from its own external data files, which it does not run data fixers on. this means the only way we could remap ender storage inventories would be to explicitly handle them in GTCE code, which may be somewhat out-of-scope

[LAGIdiot]

for posterity: the fix fails to deal with the ender chests from the Ender Storage mod. it seems to load its chest data from its own external data files, which it does not run data fixers on. this means the only way we could remap ender storage inventories would be to explicitly handle them in GTCE code, which may be somewhat out-of-scope

This is definitely out of scope and we are not responsible for problems it may cause. It was their choice to store it externally and not run fixers on it. I expect that they choose not to run them knowingly but please check if they are aware of it.

[ALongStringOfNumbers]

I expect that they choose not to run them knowingly but please check if they are aware of it.

Even if they were not aware of the issue, they are no longer doing any development work on 1.12.2, and so getting any possible fix from them, or getting a PR approved introducing a fix might not be possible.

[ALongStringOfNumbers]

In the testing done in the Omnifactory pack, besides the issue with Ender Storage, the only other currently known issue is with Little Tiles structures not getting the updated version of the block, which is a minor issue. Therefore I believe this PR should be given another review cycle so that any final issues can be found.

[LAGIdiot]

Okay then I believe that testing phase is finished and was successful base on provided feedback.

I will make code review of this as priority.

[LAGIdiot]

I have marked few place that needs adjustments otherwise this is solid

[serenibyss]

All requested changes have been addressed

[LAGIdiot]

This looks good to me know. Thanks for your hard work everyone! I will merge this immediately so it we can build on top of it.