Add hasBlockEntity to IBlockExtension for per-BlockState BlockEntities

[TelepathicGrunt]

NOTE: This PR is mostly to show proof-of-concept and to help centralize conversations surrounding per-BlockState BlockEntities. I am not personally advocating for merging this. Just want to follow through with my last comment on #1369 (comment) to make a place for these per-BlockState BlockEntities debate to be recorded to.

Background

Pre-1.17, Forge had patches in to support per-BlockState BlockEntities. However, in 1.17, Mojang made several changes to how BlockEntities are made and works. In the porting process to 1.17, Forge removed a lot of their patches for per-BlockState BlockEntities in favor of Mojang's BlockEntity interface usage. In fact as of 1.17+, per-BlockState BlockEntities are not 100% supported by neither Minecraft nor Forge/Neoforge with some edge cases from attempting to do them.

Some convo from around 1.17 release:

Per-BlockState BlockEntities problems today:

Here are the issues one will face when attempting to do Per-BlockState BlockEntities today:

• Non-BlockEntity BlockStates are not pushable by pistons

• In commands that take a block tag, BlockStateParser is going to assume the default BlockState of the block has a BlockEntity. And then will suggest { for nbt in a command for the block even if your default BlockState does not have a BlockEntity.

• WorldGenRegion#setBlock during the phase that is not ChunkType.LEVELCHUNK will create a CompoundTag with x, y, z, and "DUMMY" id to save at the spot even if the BlockState does not have a BlockEntity because hasBlockEntity returned true. This then causes ChunkAccess#getBlockEntitiesPos to report your block's position as a BlockEntity.

• LevelChunk#setBlockState would not remove the BlockEntity when switching from a BlockState with one to a BlockState without the BlockEntity when it is the same block each time.

• LevelChunk#createBlockEntity looks like it will call newBlockEntity on your BlockState without a BlockEntity to try and force create the BlockEntity. You would have to know this before-hand and return null when the state isn't what you want.

• LevelChunk#getBlockEntity which calls LevelChunk#promotePendingBlockEntity looks like it will spam Tried to load a block entity for block {} but failed at location {} if a BlockState without the BlockEntity was placed during worldgen. It checks if "DUMMY" id exist for CompoundTag and if so, will call newBlockEntity on your BlockState without a BlockEntity to try and force create the BlockEntity. If you return null from this method, then the logspam will occur.

• BlockBehavior#onRemove would not remove the BlockEntity when switching from a BlockState with one to a BlockState without the BlockEntity when it is the same block each time.

Luckily, rest of hasBlockEntity calls are paired with getBlockEntity call which returns null for non-BlockEntity BlockStates which usually would trigger the false hasBlockEntity codepath so those areas aren't actually breaking.

Per-BlockState BlockEntities pros and cons:

Pros:

• Re-use same block without needing to split it into multiple blocks and duplicate assets and setup.

• May be easier to use for certain use cases. (Use cases are not entirely clear however)

• Very small patch on Neoforge's side to fully support. (See this PR's files)

Cons:

• Not something that vanilla supported and had issues since 1.17. Yet basically no one noticed or complained about this until Neoforge was cleaning up some patches in 1.21 that had no effect on Per-BlockState BlockEntities.

• The patches could incur a small performance hit.

• Might be better to have modders split their blocks into two. One for BlockEntity and the other for when no BlockEntity.

• Might throw other modders for a loop if they are checking for presence of BlockEntity interface instead of calling hasBlockEntity. Most modders don't know that Per-BlockState BlockEntities could even be attempted and so, much of the modding ecosystem may not be setup to expect these kinds of blocks.

Please leave your feedback and stances below. Above is just a summary of the debates on #1369 and in Discord.

@KnightMiner when you port to 1.21, please use the bot below to test a build of this PR to see if it indeed resolves all the edge case issues your mod have had since 1.17.

[neoforged-pr-publishing]

• Publish PR to GitHub Packages

Last commit published: cbc03058e1226cceca49c063e538458b84d2ce7d.

PR Publishing

The artifacts published by this PR:

• 📦 net.neoforged:testframework:21.3.33-beta-pr-1409-BlockStateSensitiveChecks
• 📦 net.neoforged:neoforge:21.3.33-beta-pr-1409-BlockStateSensitiveChecks

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1409' // https://github.com/neoforged/NeoForge/pull/1409
        url 'https://prmaven.neoforged.net/NeoForge/pr1409'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1409.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1409
cd NeoForge-pr1409
curl -L https://prmaven.neoforged.net/NeoForge/pr1409/net/neoforged/neoforge/21.3.33-beta-pr-1409-BlockStateSensitiveChecks/mdk-pr1409.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

[TelepathicGrunt]

From Discord, a use case described for per-BlockState BlockEntities is:

My response is for @marchermans, couldn't your non-BE block and BE block both have the exact same blockstates? Then when converting to the BlockEntity form on multiblock match, all you do is swap the block for the other block and copy the blockstate properties over and done. The BE block will handle creating the BE for you and vice versa. Seems like a simple clean solution to just swap block and copy blockstate properties over. It would be easy to datagen the two blocks to be exactly the same too and only have 1 of them be a blockentity

[lukebemish]

I am generally against having the same block have some states be block entities and some states not -- namely, because in some instances (think commands that take a block and potentially block entity data), it's going to check the hasBlockEntity-ness of the default state for autocomplete and whatnot -- and that's not something that's easily patched or fixed or whatever, because it uses the defaultBlockState. Basically, making this per-BlockState leads to confusion any place logic is done based on the Block, not the BlockState, and the default block state is currently used to handle that. Which is both a not-really-changable case in vanilla (namely, those command inputs), as well as a case modders should expect to work given that vanilla does it.

Basically: this causes weirdness in vanilla cases that as far as I can tell cannot really be patched to not be weird when dealing with such blocks, so we should assume that similar cases may exist in mods.

[KnightMiner]

If your logic is checking the block instead of the block entity, you really should just check block instanceof EntityBlock as your question is then "does this block ever have a block entity?" not "does this state have a block entity"

For the record, I don't think vanilla ever used per state block entities, that has been a forge patch for quite a while. Originally the justification was the limitations of registry size meant it was worth compacting gameplay distinct blocks into as few registry entries as possible, and in some cases you had blocks that only needed 1 state but wanted different block entity logic. While that was the original intention.

While that was the original intention, it can be useful without this patch for a small performance gain when a block need not have a block entity if the state that is set is not using it. Doing the same thing with separate registry entries per block leads to a ton of duplicate code as we are now forced to split the block such that the block entity logic and the non-block entity logic both have their own class; this makes it an absolute pain to handle the common case I do where the block entity logic is reused but there are distinction non-block entity behaviors (e.g. I have variants of my block that have behaviors such as ladders or not having a hitbox). Splitting the block changes an if statement to entirely cloning the entire block class 2-4 times based on how messy it makes the inheritance structure (because blocks are inheritance based, not composition)

With this patch, I get the same behavior as I have now with some edge cases fixed. Most of them are pretty small edge cases that most players won't notice, but its still smoother gameplay.

---

tl;dr: its a small Neo e patch for a very useful feature for making cleaner code, all at the absolutely tiny cost that a method on BlockState now must be assumed to be block state sensitive. To be completely honest, I think you will find more modders confused by the fact BlockState#hasBlockEntity is not block state sensitive than those who will be confused that it now is. If it was not meant to be block state sensitive, I'd expect it to be on block.

[lukebemish]

I think you missed the main part of what I said: there is a vanilla place that assumes it's Block sensitive not BlockState sensitive, by using the value from the default block state. This place cannot really be patched to make it truly BlockState sensitive because, as mentioned, it's using it from the default BlockState. Thus, modders will -- rightly! -- assume that the same default-blockstate-then-check-method is valid. Breaking that assumption is bad.

If you have a way to patch the vanilla place in question I'm all ears. But given how command parsing works, I'm not sure how feasible that is. And I'm of the opinion that if something works in neo-patched vanilla code, it should work in mod code. So either it should remain only block sensitive, or the vanilla code needs to be patched to respect the full BlockState sensitivity, which -- given the context of command parsing -- seems impossible.

Basically: currently, vanilla assumes, in places like command parsing, that a Block can be uniquely determined to have a BE or no. I think it's fine to make a patch that changes that only if every vanilla place that makes that assumption is patched. And there's a location I'm not sure can be feasibly patched just given the nature of what's going on there and the fact that the only context it may have is a block ID typed in by a player in the command parsing.

[TelepathicGrunt]

The command suggestion thing is the only spot in all of Mc that is checking the default state for if it is a block entity. That’s it. Rest of Mc would be fixed with patch

[KnightMiner]

Yeah, command suggestions seem like a very minimal problem. I'd say Neo just patch it to be an instance of check in that single place and call it a day if its an issue. Does it actually cause issues if the specific state lacks a block entity or is it purely an extra argument that gets ignored?

[TelepathicGrunt]

Bad bot. it isn't as breaking as the bot says.

Tbh, this is a tiny patch to make the blockstate-sensitive blockentity work when mojang's current system is nearly there. Only a specific command suggestion will not fully work properly with blockstate-sensitive blockentities but no one is going to notice that anyway and is a tiny thing. Everything should work(tm) with this PR

[lukebemish]

I'm still unconvinced about this this, as opposed to people just making has-BE and non-BE variants of the block in question, when we no longer have issues with registry size like back in the day, duplicate logic/assets can be fairly easily avoided, especially in the day and age of datagen, and when assuming uniform block-entity-ness of a block is something mods should intuitively be able to do seeing as vanilla does it.

[neoforged-automation]

@TelepathicGrunt, this pull request has conflicts, please resolve them for this PR to move forward.

[KnightMiner]

I'm still unconvinced about this this, as opposed to people just making has-BE and non-BE variants of the block in question, when we no longer have issues with registry size like back in the day, duplicate logic/assets can be fairly easily avoided, especially in the day and age of datagen, and when assuming uniform block-entity-ness of a block is something mods should intuitively be able to do seeing as vanilla does it.

Oh, are you datagenning .java code now? The issue with duplicate logic comes down to the fact that you can't "unimplement" EntityBlock on a block, and Java does not support multi-inheritance of logic. So if you want to have identical behavior for a block with a BE and a block without a BE, they can't inherit logic from a parent that has a BE, you are forced to implement BE specific logic at each stage to get over the interface requirement.

[lukebemish]

Can't you just make the parent not have a BE? If it's being extended by both things that do and don't have a BE, that seems sensible... I get what you're saying, but these are all fairly minor solvable issues with a bit of rethinking of what inherits from what to actually make sense in the context of the blocks you have. Like, if it doesn't have a BE, it doesn't inherit the BE logic, done. If you need to store a bunch of shared for-BEs logic somewhere, use composition, or flip it and do the same for between the BE and non BE block and don't give them a common ancestor, or whatever.

[KnightMiner]

The case is not "block with BE" and "block without BE". Its "parent block with common complex BE logic", "child 1 with BE", "child 2 with BE", ... "child N with BE", "child A with optional BE".

This is trivial to handle with optional BEs. This is an inheritance mess with separate block instances to make the BE go away. Not to mention the number of areas expecting the block that now have to check for "block without BE or block with BE". Its a lot of added complexity in logic so Neo can drop a simple patch. If Neo is going to drop patches that have alternatives there are many more expensive systems to consider before this simple thing.

[lukebemish]

Sounds like that logic shouldn't be attached to that root class, then, if one of its subtypes may not have a BE...? Am I missing something here? It's like having your Lizard inherit from Mammal, you shouldn't expect that to work.

[KnightMiner]

Sounds like that logic shouldn't be attached to that root class, then, if one of its subtypes may not have a BE...? Am I missing something here? It's like having your Lizard inherit from Mammal, you shouldn't expect that to work.

Yeah, you are missing something. There are multiple unrelated behaviors that get combined into one block, as that is how mods work. Java has no multi-inheritance, so combining behaviors means inheriting or copying code assuming you wish to override methods.

In real life not everything follows linear inheritance patterns, there is a reason we like composition so much.

[lukebemish]

combining behaviors means inheriting or copying code assuming you wish to override methods.

Okay so lemme get this right. You've got group-of-things A, which are all BEs and have BR related logic. And you have group-of-things B, which are blocks and have some block specific logic. And you have a given block that's both. Isn't this an issue normally? I'm not seeing how allowing optional BE-ness fixes anything. Like, in the case where you have some root logic A, some BE logic B, and some more block-specific logic C -- wouldn't you just have C extend A, and then have the block with the BE grab that logic via composition instead of inheritance? I'd have to see your exact case but nothing here seems undoable with some refactoring.

Anyways, I see what you're saying, but it's completely solvable. And even if it wasn't -- making a less sensible inheritance tree is hardly a good solution to the actual issue.

[KnightMiner]

Like, in the case where you have some root logic A, some BE logic B, and some more block-specific logic C -- wouldn't you just have C extend A

The root object is A. The block specific logic is B extends A. There is no variant of the block that has block specific logic and lacks a BE as its not needed. Its just a single if statement to make the BE not show in that one specific case.

That said, you are woefully misunderstanding the issue as a whole. The issue is not whether its solvable. What I have was perfectly valid until Neo decided they needed to drop a feature. Its a relatively trivial API that makes the code I have work, and a relatively huge mess to make it work without this feature that already existed in Neo before. Is Neo just going to start randomly dropping features now as "its possible with way more convoluted approaches to do it without Neo" now?

The presence if a block entity is not a block defining behavior. We have since 1.7 supported block entities being block property specific. It makes many things more convenient when matching a block having non-block entity specific behavior as the one block state that means "you have a block entity" need not make every other usage of the block behavior more complex just so Neo can ditch a feature its had for over a decade.

[TelepathicGrunt]

this feature that already existed in Neo before.

@KnightMiner This feature did NOT exist in Neo before. The previous patch that existed past 1.17 was broken and literally did nothing. Zero end impact. No behavior change for anything or anyone lol. And it seemed that way for a long time.

In Forge up to 1.17, it was supported use case. After 1.17, the original main patch was erroneously removed as it was thought vanilla supported it but the patch in the other PR remained for some reason. Likely was missed. That "patch" was broken and the logic literally did nothing.

So this is more of a new feature if anything. Or well, re-implementation of the pre-1.17 use case from Forge into NeoForge now. From 1.17 to 1.21.4 now, per-BlockState BlockEntities was not supported.