Minecraftforge: Block#isEntityInsideMaterial() method somewhat screwy

[jabelar]

Okay, so the Block class has an isEntityInsideMaterial() method. Based on the comments for the method it seems that it was intended primarily for things like liquids/fluids. The description says:

  * Called when the entity is inside this block, may be used to determined if the entity can breathing,
  * display material overlays, or if the entity can swim inside a block.

However, even though the return type is boolean, the default returned value is null (not great programming practice in my opinion). I don't know the history of the decision to return null, but it seems to be because of a forge hook (I'll discuss later) that they want to fall through. But that is the beginning of where it gets screwy IMO.

Now the first big problem is that even though the method is called in two places (from the Entity#isInsideOfMaterial() method and the World#handleMaterialAcceleration() method) the method is never overridden by any class. Basically it is unused in vanilla Minecraft. Even classes you'd expect like BlockLiquid or BlockFluidBase don't use it .. more on this later.

Now, there are lots of examples of orphaned, unused methods in Minecraft. But this one is potentially very useful for liquids and fluids. In fact, it was during my investigation on some of the remaining deficiencies in the Forge Fluid system where I thought it would be useful to use this.

If you have worked with fluids, you'd know that unless you use Material.WATER for your block, you don't get any of the water-like functionality you probably want for your fluid, such as swimming, boating on top, swimming, drowning, and such. Material.WATER is hardcoded throughout the vanilla code. And using Material.WATER for your fluid block has side effects like the map color can only be blue.

In fact, as a test by simply overriding the isEntityInsideMaterial() so that when water was being checked I returned true if I had a Material that returned true for isLiquid() I was able to get all the water-like functionality into my fluid but using my own material.

I would just go ahead and make a Pull Request to implement this for everyone, but there is one catch -- the null return value. If I return true, then there is a code path that is skipped. The Entity#isInsideOfMaterial() method has the following code:

` Boolean result = iblockstate.getBlock().isEntityInsideMaterial(this.world, blockpos, iblockstate, this, d0, materialIn, true);
if (result != null) return result;

        if (iblockstate.getMaterial() == materialIn)
        {
            return net.minecraftforge.common.ForgeHooks.isInsideOfMaterial(materialIn, this, blockpos);
        }
        else
        {
            return false;

`
So you see that if I return true it will skip the opportunity to do the forge hook isInsideOfMaterial(). So what is that method? Well you can look yourself, but lo and behold it is related to fluids and is basically trying to figure out if the entity's head is below the level of the fluid (similar to how water does it).

But this is all pretty screwy. I'm guessing that when they added the fluid system they wanted to do the fluid level checking but that caused them to allow for a possibility for a null boolean value from the block method.

Proposed fix: I believe that the forge hook code that checks for fluid blocks should simply be put into the isEntityInMaterial() method within the BlockFluidBase itself. Why do this weird check of the block which does nothing but returns null and then check again whether you're that block and call another method? Furthermore as cleanup I think the default value from Block should actually return true if you're in the material.

Sorry if the above explanation is a bit hard to follow. Summary is there is an ophaned method that is potentially useful and people have made a forge hook method to do something that logically should be in that method.

Does anyone know the history of this method (I'll try to search change log and PR history more to see if I can dig it up) and why these decisions were made? I think anytime you're puposefully returning null for a boolean, you should probably check if there is a cleaner way ...

[jabelar]

One other thing I should say about the use of methods where you ask if it is in a specific material -- this is very limiting to modding because even if you extend the material you don't get a match and lose out on functionality. It would be much preferable to directly check a property of the Material rather than exact match the Material instance. For example, if you want to know if it is a liquid, use the isLiquid() method rather than just checking for the known MaterialLiquid instances: WATER and LAVA.

So the alternate fix would be to continue to let these methods be orphaned and instead do PR patches that do more precise but modding-friendly checks based on material properties.

[mezz]

Here's the original PR that created the method: https://github.com/MinecraftForge/MinecraftForge/pull/2429
As far as I know it was created for chisels and bits liquids.

Vanilla checking for == Material.WATER is completely awful but it's not something we can easily fix without a ton of patches, so we just occasionally complain to whoever will listen at Mojang.

[jabelar]

Here's the original PR that created the method: #2429

Perfect, thanks. So yeah it was definitely intended for a similar sort of intention, but I think it is weird how it was not really integrated into the fluid system but rather tries to work around it with a null return value.

Vanilla checking for == Material.WATER is completely awful but it's not something we can easily fix without a ton of patches

It does permeate through a lot of code, but I think it could be made fairly tight if most of the logic was put into some forge class instead. Imagine there were boolean forge hook methods something like isInPushingLIquid(), isInDrowningLiquid(), etc. that handled WATER and fluids (and fluids could allow individual control over these).

With such methods, the patches would consist of one-liner replacement of the appropriate method. The isInsideMaterial(Material.WATER) in the place where drowning happens would be simply changed to isInsideDrowningLiquid().

The number of patches depends on which functionalities we want to implement. But it would be a about a dozen lines for the main things like drowning and pushing.

[jabelar]

Okay, assuming that it IS too much work to replace all the references to water, it is still quite easy to make a boolean that make Fluids have the ability to get all the features of water without having the same material (this solves the map color problem). Wouldn't get to select the individual aspects of water-like behavior, but maybe that is okay.

[mezz]

Please take a little more time to edit your thoughts down to something a bit more concise...

I think if you have a specific thing in mind here, you should PR a fix for it specifically.
The general solution of fixing all the Material.WATER references is most likely a large undertaking that is not simple to maintain across Minecraft version updates. We should avoid it, unless there is a very compelling reason and you think I am overestimating the scope of it.

[jabelar]

I actually have very concise ideas. In fact I already have it fully implemented. I'm just not sure whether other people have a strong opinion on it. Figured it was easier to discuss it as an "issue" rather than go through the work of people reviewing and actual PR.

I guess I'll just do a PR for the way I would prefer to do it and let the discussion flow from that then.

Regarding the work related to replacing water behavior it is a matter of just dialing in how much water behavior is of interest. Each aspect is usually just one line of code. So if we just want to have fluids push like water and drown people, we can leave it at that. I agree that covering all the behavior is more involved, but that isn't really what I'm proposing. I was able to get all the behavior I wanted with about 12 lines of patched code.

Regarding maintenance, yes, that is a factor for every forge feature. I don't know how many people are really using the fluid system (it would be cool to have a way to measure how many modders use each forge feature to help determine how much work should be put in), but all I know is that if you do use the fluid system it is disappointing when your fluid can't do any of the stuff water can do...

[williewillus]

I think the request is, in other words, can you reduce your 11-paragraph first comment into a concise bullet list of problems and solutions that people unfamiliar with the method can understand?

also, null Boolean returns are common in patched in methods when both true and false have defined non-vanilla semantics, and null is used to fall back to vanilla. I'm not sure how true that is in this case as I'm not familiar with this patch

[mezz]

That sounds fine jabelar, go ahead.

[jabelar]

Okay, I submitted PR #4478 to address this, along with a number of other Fluid behavior features.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

[jabelar]

Closing since I have submitted PR to take care of this.