Mod Compatibility: FramedBlocks

[Erysch]

FramedBlocks: https://www.curseforge.com/minecraft/mc-mods/framedblocks

FramedBlocks adds blocks of various shapes that can be made to look like almost any other block in the game.

Usage Right click the block with almost any solid block to make it look alike. Right click the block with glowstone dust to make it produce light.

In case you don't know this mod, it adds several different shaped placeholder blocks which can 'absorb' the texture from the block you are placing in it (and using it up). Currently it's only possible for the Gadgets to copy and place these FramedBlocks without any 'absorbed' textures (it might had before), meaning that you manually have to put blocks in these after you maybe placed a line of 14 blocks of these with the Building Gadget. It would be a huge game changer in some cases if it would be possible to copy and paste these blocks with their respective textures (using up the FramedBlock + the block item for the texture).

I really appreciate your mod since it made building actually fun again when trying to build huge projects. Will this proposal be worthy of your implementation?

Greetings

[MajorTuvok]

@XFactHD actually there is a kind of api there that you could use to carry over nbt if you want to (which I presume you're using to store the contained block, right?). However @MichaelHillcox is also working on a rewrite

[XFactHD]

The problem is that I would not only need to carry over the NBT data that stores the contained block and wether the block has glowstone applied, I also need to consume items from the player inventory and either stop carrying over the NBT if I can't do that or stop the tool from placing the blocks (might be better for user experience when they don't have to destroy part of the placed blocks again when they ran out of the additional materials).

[MajorTuvok]

@XFactHD you can specify the required Items (though not experience) which results in the tool counting your block to need the additional items. If necessary you can also override the placement procedure to do some custom stuff that is potentially required by your blocks...

When I designed this I had cases like this in mind - however as I've been told this is too complex which is why it is marked as Tainted and scheduled for removal in the rewrite. Ripping out the tranaaction System in a hurry also didn't make it better :(.

(I still like my old design even though I would go differently for the Inventory Management now, but sigh... It needs to be simpler)

[to be precise inventorx mangagement could also be done in O(n+m) time but with smaller constants than atm if a single linear search is performed over the inventory and an index of callbacks is created that are subsequently when an item is found that matches something in the index...]

[MajorTuvok]

See https://github.com/Direwolf20-MC/BuildingGadgets/blob/db98d7f46a046788bed9166f0ee9d3752bfda2e4/src/main/java/com/direwolf20/buildinggadgets/common/tainted/building/tilesupport/ITileEntityData.java#L24 For an overview of what can be done, though the docs is not 100% accurate (attemptPlacement has been ripped out)

[XFactHD]

Thank you, I will look into implementing this in FramedBlocks. Are there already plans when this will be replaced? I am mostly interested in whether I can expect potentially breaking changes in 1.16.

[MichaelHillcox]

I don't think I'll be making breaking changes to 1.16 anymore as 1.17 and .18 will be the new focus. Likely lots of breaking changes in 1.17. Already got the mod ported, just fixing all the bugs, then lots of code changes to come

[XFactHD]

Perfect, thanks for the heads up

[XFactHD]

I have tried to get this to work for a few days now and I found a few issues while implementing it.

---

The first thing I noticed is that the default implementation of ITileEntityData#placeIn() has a major issue: it passes the NBT data to the new TileEntity without replacing the stored block position with the position of the new TileEntity for every block that is placed. Instead the tag still contains the position of the copied block. This leads to the data not applying to the TileEntities.

---

The second thing I found after dumping the stack NBT of the Building Gadget because the deserialized MaterialList was always empty is that the read and write methods of the MaterialList are missmatched. MaterialList.writeEntry() writes the serializer name and the tag containing the NBTList to the result tag and produces the following structure:

data: {
    data: [{
        <ENTRY>
    },
    {
        <ENTRY>
    }]
},
serializer: "buildinggadgets:entries"

MaterialList.readEntry() reads the serializer from the passed tag and passes the incoming tag directly to that serializer instead of getting the "data" tag from it and passing that. It expects the tag containing the NBTList to be placed on the same level as the serializer name instead of one level deeper:

data: [{
    <ENTRY>
},
{
    <ENTRY>
}],
serializer: "buildinggadgets:entries"

My workaround for this is to copy the serializer name into the outher "data" tag and passing that "data" tag to MaterialList.deserialize() in my custom ITileDataSerializer.

---

The third problem I encountered and the only one I have not found the cause let alone a workaround for is that blocks that change state depending on their surroundings (walls, fences) behave extremely weird. Copying blocks with this behaviour leads to the following results: From left to right:

• (1 and 2)Framed Walls don't connect to existing blocks (the existing block needs to be replaced to make the connection) but the existing block creates a connection to the newly placed block and only the first Framed Wall that is placed actually gets the data
• (3 and 4) Framed Fences behave exactly the same as Framed Walls
• (5 and 6) Framed Lattice blocks don't connect to existing blocks (in this case it looks like the first granite colored Lattice connected to the diorite colored Lattice but this is caused by the second Lattice placed by the Building Gadget. If only a single Lattice is placed then no connection will be made), existing Lattice blocks connect on the server but that state change is not synced to the client and every second Lattice gets the data. To create the connection the existing block needs to be replaced
• (7) Vanilla walls and fences don't connect to existing blocks, the existing block needs to be replaced to create the connection

The materials needed for the camo of all blocks is still consumed (which makes sense). Relogging only fixes the desync issue with the existing Lattice block

[MajorTuvok]

Thanks for the catches and sorry for the errors. Though fixing these should not take long - I'm writing my last exam for this sememster in exactly one week (whilst I do have a seminar presentation still ahead of me, I think the first two should be a quick fix right and thus not conflict with that) - so I would quickly write a PR for these 2 things

[MajorTuvok]

However not sure if fixing that would be a good thing for the serializer Problem, as people like you would have to write an extra if to work with the erroneous case etc.

[XFactHD]

Agreed, fixing the serializer would probably be a breaking change and it's easy to work around.

[MichaelHillcox]

I don't think many mods use this system, come 1.17 - 1.18, I'll likely have a lot of these systems changed and cleaned up. I'd have happy to just fix the derp :P

[XFactHD]

I have looked further into this and found the root cause of the missing TileEntity data:

1. The Building Gadget places a ticking TileEntity
2. The animation plays
3. At the end of the animation, the EffectTileEntity places the target block in its position
4. The World checks if the new BlockState has a TileEntity and if it does, it creates it and tries to add it
5. Because the World is currently ticking TileEntities it adds the new TileEntity to a secondary list (World#addedTileEntities) instead of the primary list (World#loadedTileEntities) and the Chunk's TileEntities. This is to prevent a CME.
6. The World now calls AbstractBlockState#updateNeighbours() which in turn calls BlockState#updatePostPlacement() on all direct neighbors
7. The state change caused by this leads to the new state being set in the World at the neighboring position. In this process the Chunk checks if it has a TileEntity for that position but it doesn't due to the World not adding the TileEntity to the Chunk when initially placing this block
8. Therefore the Chunk now creates a new TileEntity and calls World#setTileEntity() which (due to the CME protection) adds it to the secondary list (World#addedTileEntities) and before doing so removes all TileEntities that have the same position as the new one from that list thereby deleting the TileEntity that ITileEntityData#placeIn() wrote to earlier
9. After all TileEntities are ticked (and therefore all EffectBlocks are replaced with the respective target block), all TileEntities in the World#addedTileEntities list are transferred over to the World#loadedTileEntities list

This cycle (1 to 8) "walks" down the line of blocks away from the player. The last block is not affected and has the proper data because there is no block that would call Block#updatePostPlacement() on it before the ticking is done. The call to World#getTileEntity() in ITileEntityData#placeIn() doesn't have the problem that Chunk#setBlockState() has because World#getTileEntity() checks the World#addedTileEntities set if the World is currently ticking TileEntities.

---

I have found two ways to fix this:

• Set a specific bit in the flags passed to World#setBlockState() to omit neighbor updates (Constants.BlockFlags.UPDATE_NEIGHBORS). This is probably not the desirable solution though because the player has to do additional work to get the walls to connect for example
• Delay writing the data to the TileEntity until the end of the current server tick with a TickDelayedTask:

  IWorld world = context.getWorld();
  if (world instanceof ServerWorld)
  {
  MinecraftServer server = ((ServerWorld)world).getServer();
  server.enqueue(new TickDelayedTask(server.getTickCounter(), () ->
  {
      //Get TileEntity, set BlockPos in NBT, send to TileEntity
  }));
  }

---

Regarding the newly placed blocks not properly connecting to existing blocks (like in the case of walls), I might call defeat on that and leave it up to you to decide if its important to fix

[MajorTuvok]

Thanks for all the effort

[MichaelHillcox]

I think scheduling the TE placement is like the best thing, we only ever merge data so delaying it a bit and letting the block setup properly will likely do nothing but good for supporting TEs. As for walls, they're a pain, I'm going to look through how the structure block handles them when placing to see if I can get any clues from how they place blocks like that and keep the connected. Are you walls custom ones or are they just generic ones as I though default walls and fences did join after placement or are we talking about construction paste here?

Thanks again for the detailed write up and the code examples, this'll go a long way to fixing the root issues here, I'm going to try and find some time to put into BG in the coming days as the mod is not in a good state and it makes me sad haha.

[XFactHD]

Are you walls custom ones or are they just generic ones as I though default walls and fences did join after placement or are we talking about construction paste here?

My walls extend from vanilla walls. Walls and fences do join on placement and on neighbor placement. The reason it doesn't work properly here is that the connection on placement is done in Block#getStateForPlacement() which the Building Gadget doesn't call (it places exactly the state that was copied) and the connection check on neighbor placement (Block#updatePostPlacement) only checks on the side that is passed into this method by the neighboring block

[MichaelHillcox]

So would the best solution be for us to get call that method? It's very hard for us to call a method like that because we don't know what side we should be telling it to connect on, and with most blocks like walls and fences, as long as it's not the top or bottom, it shouldn't really care about a direction

[XFactHD]

My understanding of the Building Gadget is that it is supposed to place blocks with the exact state that was copied (atleast for the most part). For that reason I don't think calling Block#updatePostPlacement() is a good idea. If you want to call it, you would probably need to call it for atleast the 4 cardinal directions if not all 6 directions. The vanilla walls and fences are extremely picky about the side you pass in, walls even check above them if UP is passed in because they adapt their height depending on the block above.

[MichaelHillcox]

If you wanna just have a chat, you can join our dev discord https://discord.gg/qqUUtu3 might be a bit simpler than chatting in here