Create Schematicannon places malformed DO blocks that throw NPEs when trying to break them
TemporalWolf opened this issue · 15 comments
The issue with the schematicannon has already been reported to Create, but the NPE seems to be on Domum's side.
java.lang.NullPointerException: Cannot invoke "net.minecraft.world.level.block.Block.getSoundType(net.minecraft.world.level.block.state.BlockState, net.minecraft.world.level.LevelReader, net.minecraft.core.BlockPos, net.minecraft.world.entity.Entity)" because "block" is null
at com.ldtteam.domumornamentum.block.decorative.PanelBlock.getSoundType(PanelBlock.java:164) ~[domum_ornamentum-1.19-1.0.83-ALPHA-universal.jar%23507!/:1.19-1.0.83-ALPHA] {re:classloading}
Full crash log:
https://gist.github.com/TemporalWolf/f1227a738853789b0ccf95541d72dbb9
There is not really something we can do. The cannon places an invalid block without data.
What do you expect to happen when you break it?
What sound do you expect to play when you start breaking it?
Yes I know it should not crash, but does not happen in normal gameplay, just when the block was placed without valid data.
The default texture is oak planks, so I would expect the sound of oak planks, of no sound at all.
Note that the same can happen when someone accidentally placed a panel from the creative tab menu or JEI, as those block's don't have data either
The schematic server also had an issue where DO blocks got corrupted somehow, and it would be nice being able to correct those without crashing each time (for at least the few we missed).
In survival, this cannot happen indeed without the uses of the schematicannon, or other block place systems which don't set the data properly. In creative this is quite easy to run into
So, there are two things that can be done on the DO side:
- As a compatibility feature, if create is also installed, add all relevant DO blocks to the
#create:safe_nbt
tag. - Fix the NPE on interacting with blocks with invalid data, which can happen even without other mods.
Also I think a Create bug should be opened that a lot more blocks should have their NBT saved in schematics, because this affects even vanilla and Create-specific blocks (although they donʼt crash as far as Iʼve seen, just lose inventories or display incorrectly or whatever).
As a workaround, /setblock <x> <y> <z> air
will destroy the corrupt block without trying to find a sound from nonexistent NBT data.
So, there are two things that can be done on the DO side:
- As a compatibility feature, if create is also installed, add all relevant DO blocks to the
#create:safe_nbt
tag.- Fix the NPE on interacting with blocks with invalid data, which can happen even without other mods.
This issue was opened because of point 2. Not just because Create causes it, but because corrupted blocks can also be caused in other ways, even with just DO installed
Yeah while investigating a different issue I made blocks with /setblock
, and they crashed my game unless I was careful. I expected them to either crash immediately (not ideal, but I felt a reasonable guess) or never (ideal case), not a delayed crash only when I interacted with them.
I do think 1 would also be a good idea, because lots of people seem to use the two mods together so itʼd be nice to have some compatibility, 2 should definitely be handled.
So the Null Exception is fixed on the current release 1.0.110. But the blocks loosing their textures, when moved.
Configuration
- Minecraft: 1.20.1
- Forge: 47.1.42
- Mods: only Domum 1.0.110
Reproducing the Bug
- Build two material block in architecture cutter (currently only tested with framed, doublecrossed, ... material doesn't matter)
- place a piston in lying position
- place the the DO block in front of the piston
- activate the piston, so that the block is moved by one
- save map & load chunk file
Example NBT
- I placed three blocks (two with cobble stone + acacia frame, one with double dark oak combo)
- Moved the middle block using the piston
- saved the game
Unfortunatly I don't really know, how the block movement is implemented, but i looks like, that the MateriallyTexturedBlockEntity is resettet / replaced with a quite new version with default values. (assumption)
When the movement starts the block changes immediately to the default textures dark oak + oak frame.
I analyzed the way how Domum creates and saves block data and this behaviour comes "unfortunately" out of the way of handling textures. To describe it simple: Minecraft uses an single step block creation process and most things were more or less static. To by flexible in case of textures and to handle everything efficiently, Domum needs to go another way and has two-step init process, which is realized by the architecture cutter, which creates itemstack / block initialy with the seen textures and updates it after the creation to the correct textures from the used materials in the cutter.
What has that to do with the seen behaviour?
The root cause seems that a movement of the block inside the world creates "faulty" blocks. The crash is currently fixed since 110 by simply check the object and not returning "null". What stays is the default textured / faulty block. Some details out of my "Piston based Testing scenario":
- When a piston moves a block, the block needs to be "moved" at end of the extraction process of the piston from initial postion in the map to the 'extracted' position
- Minecraft realizes this movement in the way: block.Remove(oldPos); block.create(newPos)
- With this behaviour only the minecraft 'first stage' were used to create the new block, which has default textures and some missing feauters, which will set by the domum 'second stage'
- Result: we see flipping textures on the block, because it's a new one
How Minecraft solves this Problem
- Define complex entity block objects as "unmoveable" (i.ex. Chests, ...)
Idea to fix this Piston based topics
Each block entity is marked in update process of the level "to be removable". At the point we can overwrite this behaviour to have a hook up point. Next is to examine why the block should be removed: look through to caller stack for "PistonBaseBlock". When the Piston "moves" us, then save the 'textureData' with PosData and current tick.
Later in the block "recreation", which should be in the range of 1-3 ticks, we can have a look if there were an "saved entry" in a position near the new one, so that the old texture data could be reused. In theory this should work.
The idea had some negative aspects: we need to store information outside the minecraft functional behaviour, which will be deleted when minecraft crashes. Another aspect is, when i place two pistons directly in the neighborhood, i will need more information about the new position to differentiate between both pistons and their moved blocks. And last: i need a clean up everything from time to time, so that saved textureData is skipped after some ticks, when they were not used.
I analyzed the way how Domum creates and saves block data and this behaviour comes "unfortunately" out of the way of handling textures. To describe it simple: Minecraft uses an single step block creation process and most things were more or less static. To by flexible in case of textures and to handle everything efficiently, Domum needs to go another way and has two-step init process, which is realized by the architecture cutter, which creates itemstack / block initialy with the seen textures and updates it after the creation to the correct textures from the used materials in the cutter.
What has that to do with the seen behaviour?
The root cause seems that a movement of the block inside the world creates "faulty" blocks. The crash is currently fixed since 110 by simply check the object and not returning "null". What stays is the default textured / faulty block. Some details out of my "Piston based Testing scenario":
- When a piston moves a block, the block needs to be "moved" at end of the extraction process of the piston from initial postion in the map to the 'extracted' position
- Minecraft realizes this movement in the way: block.Remove(oldPos); block.create(newPos)
- With this behaviour only the minecraft 'first stage' were used to create the new block, which has default textures and some missing feauters, which will set by the domum 'second stage'
- Result: we see flipping textures on the block, because it's a new one
How Minecraft solves this Problem
- Define complex entity block objects as "unmoveable" (i.ex. Chests, ...)
Idea to fix this Piston based topics
Each block entity is marked in update process of the level "to be removable". At the point we can overwrite this behaviour to have a hook up point. Next is to examine why the block should be removed: look through to caller stack for "PistonBaseBlock". When the Piston "moves" us, then save the 'textureData' with PosData and current tick.
Later in the block "recreation", which should be in the range of 1-3 ticks, we can have a look if there were an "saved entry" in a position near the new one, so that the old texture data could be reused. In theory this should work.The idea had some negative aspects: we need to store information outside the minecraft functional behaviour, which will be deleted when minecraft crashes. Another aspect is, when i place two pistons directly in the neighborhood, i will need more information about the new position to differentiate between both pistons and their moved blocks. And last: i need a clean up everything from time to time, so that saved textureData is skipped after some ticks, when they were not used.
This has been discussed internally many times. DO does not support moving of its block or it's attached data. Full stop.
DO simply stores its block data in it's entity. If the schematic canon can not properly place a block with entity data attached, then I am sorry but there is 0 that we can do about this.
This has been discussed internally many times. DO does not support moving of its block or it's attached data. Full stop.
DO simply stores its block data in it's entity. If the schematic canon can not properly place a block with entity data attached, then I am sorry but there is 0 that we can do about this.
Sry don't know about that. A little 'but'. When this is the design descission you had made, then the consequence should be, that the block must be created with 'ignore' as 'reaction on pistons'. Currently is 'push only', which cause the defect blocks, when moved by a piston. At least this behaviour could be avoided by changing the properties.
This has been discussed internally many times. DO does not support moving of its block or it's attached data. Full stop.
DO simply stores its block data in it's entity. If the schematic canon can not properly place a block with entity data attached, then I am sorry but there is 0 that we can do about this.
Sry don't know about that. A little 'but'. When this is the design descission you had made, then the consequence should be, that the block must be created with 'ignore' as 'reaction on pistons'. Currently is 'push only', which cause the defect blocks, when moved by a piston. At least this behaviour could be avoided by changing the properties.
I will debug it, but they should not be moveable by piston at all... I will investigate