neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.04k stars 134 forks source link

Lava and fence gates work differently in NeoForge/Forge causing farms to fail #1201

Closed JalapenoGremlin closed 11 hours ago

JalapenoGremlin commented 1 week ago

Minecraft Version: {Minecraft version} 1.20.1 1.21.0 NeoForge Version: {NeoForge version. Version number, not latest/rb} 1.20.1-47.1.106 21.0.42-beta Logs: {Link(s) to GitHub Gist with full latest.log and/or crash report}

Steps to Reproduce:

  1. Build a "lava containing structure"
  2. Wait
  3. Fences and Signs burn but not in Vanilla/Fabric versions...

Description of issue: If you build a Lava holder, the wood fences/signs burn in Forge and NeoForged but not in Vanilla Or with the fabric loader

2024-06-28_23 55 53

JalapenoGremlin commented 1 week ago

2024-06-29_00 01 05

Here is what happens with NeoForged

Lolothepro commented 6 days ago

https://github.com/MinecraftForge/MinecraftForge/issues/8449 https://bugs.mojang.com/browse/MC-109948

TelepathicGrunt commented 5 days ago

Since technically this is a vanilla bug, I'm not sure if we really want to "fix" it.

Especially as normal Fire will burn the blocks but Lava doesn't set them on fire. We had to fix it when we added the flammable hooks for modded blocks to use. Not sure we really want to duplicate the hooks with one for "fire" behavior and one for "lava" behavior.

Earthcomputer commented 2 days ago

This is a commonly used vanilla behaviour, and despite the fact this hasn't been closed on Mojira, I don't think Mojang is inclined to fix this at least for the case of signs and fence gates for this reason, which is presumably also why it's been open for 8 years with low priority. Don't let this become another slabs on chests situation. Just add an exception to match the commonly used vanilla behaviour. If Mojang does change their mind then you can always remove that exception again.

TelepathicGrunt commented 2 days ago

Earthcomputer, it’s not just a quick exception as far as I can tell. It’s that vanilla checked flammability by two completely different ways. But forge and neoforge added patches for mods to set flammability for their blocks in one neat spot.

Which behavior does that flammability api work on? And if split, won’t that confuse modders into why there’s two different flammability apis?

Earthcomputer commented 2 days ago

I'm not saying expose two APIs to modders, I'm saying make an exception in those places to vanilla blocks.

XFactHD commented 2 days ago

This is vanilla behavior in 1.21. All fences, fence gates and signs (among a lot of other wooden blocks) except crimson and warped variants are marked as ignitable by lava. The lava patch checks that property: https://github.com/neoforged/NeoForge/blob/b1ffaf32b853f12e909315bf7362ba7e194062c9/patches/net/minecraft/world/level/material/LavaFluid.java.patch#L49

Surrounding the lava in just signs doesn't cause anything to burn, neither in a NeoForge environment nor in vanilla, because there are no valid surfaces for fire to sit on: 2024-07-04_19 29 01

Surrounding the lava with fences and fence gates leads to everything burning down in both NeoForge and vanilla 1.21 (any signs on the fences just pop off once the fence is gone and don't burn for the same reason as above): 2024-07-04_19 36 53

All four cases were tested with randomTickSpeed set to 3000 to make sure the result is conclusive (the second screenshot was made at 300 so I actually have time to press the button before everything is gone 😛).

TelepathicGrunt commented 2 days ago

Finally at my computer. Checking original setup picture from @JalapenoGremlin, and setting random fire tick to 5555 is not burning down the setup. image

@JalapenoGremlin Did you forget the sign above the fence gate when testing neo

JalapenoGremlin commented 2 days ago

This is vanilla behavior in Vanilla 1.21. All fences, fence gates and signs (among a lot of other wooden blocks) except crimson and warped variants are marked as ignitable by lava. The lava patch checks that property:

I have not seen this to be the case in 1.21 when lava is bounded by 3 signs (on the wall) and a fence gate is used to bound the lava on the "open side" WITH a sign above the open fence gate. All 4 signs are needed

JalapenoGremlin commented 2 days ago

Finally at my computer. Checking original setup picture from @JalapenoGremlin, and setting random fire tick to 5555 is not burning down the setup. image

@JalapenoGremlin Did you forget the sign above the fence gate when testing neo

Nope. All four signs are used. (the sign is gone in my image because the gate burned and the sign popped off into the lava)

There is one difference that I will check... It might be that the block behind the 3 signs are dirt/grass blocks (no grass or tall grass on them) in one and stone in another....

JalapenoGremlin commented 11 hours ago

Okay. I did some more testing....

It IS a problem in 1.20.1 (FORGE and NeoForge)

However as @TelepathicGrunt pointed out, it is "fixed"/"on parity" with Vanilla in 1.21.

So.... As far as I'm concerned after having done some additional testing, this is not an issue in NeoForge.

And Porting the fix back to 1.20.1 is probably beyond necessary. I have not tested the later 1.20.[2..6].

I could have sworn that I tested in 1.21 (but I should probably cull my instances in my launcher).

Unless the developers fell otherwise, I am fine with closing this as a "will not backport" fix

JalapenoGremlin commented 11 hours ago

Final comment..... It is NOT an issue in 1.20.6.

So unless someone wants to binary search for which release "fixed"/"parity set" between 1.20.1 and 1.20.6, my suggestion is to close this issue.

Thank you to everyone who answered so quickly to this issue.

sciwhiz12 commented 11 hours ago

Closing as stated above. Anyone interest in backporting the fix, if that is indeed possible in a non-breaking way, should first raise the issue in the Discord server.

TelepathicGrunt commented 10 hours ago

Apologize, I was looking at wrong thing last comment. It appears this is the PR that fixed the issue in 1.20.2 https://github.com/neoforged/NeoForge/pull/244 image

Which was a port of a Forge 1.20.2 PR for the same issue

JalapenoGremlin commented 10 hours ago

Apologize, I was looking at wrong thing last comment. It appears this is the PR that fixed the issue in 1.20.2 #244 !

Which was a port of a Forge 1.20.2 PR for the same issue

Wow. Thanks for checking the git commits.