[RFC] Changes to fire and block interactions.

[HenryLoenwind]

This is about https://github.com/neoforged/NeoForge/blob/1.21.x/src/main/java/net/neoforged/neoforge/common/extensions/IBlockExtension.java#L651-L674 as reported by MehVahdJukaar on Discord, https://discord.com/channels/313125603924639766/852298000042164244/1274689911030681661

---

My analysis:

The onCaughtFire() callback looks bit-rotten to me. It is called

• when a block is directly in front of a dispenser that uses flint&steel and
• when a block is destroyed by a fire that has been burning next to it for a while.

Those are two very different things, and neither sets the last parameter to anything but null.

Also, "this is called when it gets lit on fire" applies to the first case, but not the second one and there are plenty of cases missing one would consider "lit on fire"...

Furhermore, blocks that react to flint&steel would handle ItemAbilities.FIRESTARTER_LIGHT, which is checked first, so the onCaughtFire() from the dispenser would never fire for them. This only leaves the "on destroyed by fire" case. It's a useful case, as it's the only chance the block has to drop an item, but that doesn't match the callback name or javadoc at all.

Then the javadoc of getFireSpreadSpeed() is a bit misleading. "when fire is updating on a neighbor block" actually means "when a fire is contemplating spreading to a neighbour block". The neighbour block is not a fire when this is called, it may or may not become a fire after.

---

Proposal:

1. Deprecate onCaughtFire() for deletion with a note to use onNeighourChange() instead. (Mods need to do that at the moment anyway as onCaughtFire() doesn't do what it promises.)
2. Add a new onReplaceByFire() callback to be called by FireBlock.checkBurnOut() (see below)
3. (optional) Add a new event to FireBlock.checkBurnOut() that is posted after onReplaceByFire() with the same semantics.
4. The callback in DispenserBlock.registerBehavior(Items.FLINT_AND_STEEL gets no replacement, it already is handled by the ItemAbility.
5. Update the javadoc of getFireSpreadSpeed() to make clear it doesn't indicate that the neighbour actually is a fire.

    /**
     * This is called when a block is about to be destroyed by a neighbouring fire.
     * <p>
     * This gives the block a chance to drop an item or change what block should be set in the world as the result of it burning up.
     *
     * @param state     The current state
     * @param level     The current level
     * @param pos       Block position in level
     * @param direction The direction that the fire is coming from
     * @param newState   The state the fire wants to set. In vanilla, this is either a state of BlockFire with an appropriate age or AIR.
     * @returns the state that should be set.
     */
    default BlockState onReplaceByFire(BlockState state, Level level, BlockPos pos, Direction direction, BlockState newState) {
        return newState;
    }

[MehVahdJukaar]

It would also be nice if there was an event that fires whenever fire consumes any block so that mods could alter how other blocks are consumed

[HenryLoenwind]

And there's more...

Regarding the implementation of getFireSpreadSpeed() and getFlammability(), I see a couple of issues:

• There is no way of changing a block's flammability if that block is using neo's addition. One cannot make a mod that makes a block from another mod fireproof.
• The vanilla FireBlock is written in a way that allows one to instantiate a second copy of it and make more fire types that burn different blocks. Neo's patches disable that ability. To make a new type of fire, you now need to (subclass and) copy the complete code of the FireBlock. Overriding the accessors is not enough; you need to actually copy the logic code. (Really? For once, vanilla properly isolated code and data access...)

---

Suggestion:

1. Change the patches in FireBlock to use level+position-aware versions of getBurnOdds() and getIgniteOdds() instead of calling the blockstate methods directly.
2. Have getBurnOdds(state, level, pos) and getIgniteOdds(state, level, pos) check this.burnOdds/igniteOdds first (edit: then the data map) and only call the blockstate methods if there's no entry.
3. Change the default implementations in IBlockExtension to return 0/false.
4. Route isFireSource() through a similar local accessor.
5. Do the same for isFlammable() and deprecate it for removal. (This depends on the change to onReplaceByFire() outlined above as that can cover the "can burn but is not consumed" case "isFlammable() != getFlammability()>0" currently covers.)
6. Edit: and then move the static init to a datamap. The datamap would then be registered to the FireBlock instance, allowing other instances to use their own datamap or API-compatible supplier. With the datamap in place, the other callbacks on Block would be deprecated, too.

This change would preserve the basic functionality with one (wanted) exception: Setting a block's flammability using FireBlock.setFlammable() overrides the data from the block's callback methods.

Additionally, it again allows subclassing the FireBlock to make different kinds of fire, even though it now requires overriding the accessors (in vanilla, you only need to call the new block's setFlammable() to register the blocks you want to burn).

This would be a non-breaking change.

[MehVahdJukaar]

A bit out of scope but could flammability and fire spread be driven by data maps? Could make them more accessible for datpack people but I guess more annoying for mod Devs, assuming a new one would need to be created per new fire block. Also would be nice if fire logic would be modified such that it would allow to set non air blocks on fire. Currently get ignite odds returns 0 if target position isn't air. Imagine this could be used to say make fire ignite csmpfires

[HenryLoenwind]

A bit out of scope but could flammability and fire spread be driven by data maps?

Yes. I was thinking about having a default data map and a setter method on the fire block. That way other instances could have their own data maps (or registry-API suppliers). But for that I have to look into data maps, which I haven't yet. Still this is compatible with the changes I proposed above, as it would only replace the "check this.burnOdds/igniteOdds first" part with a data map lookup and deprecate the block methods. (the latter would be really breaking, the former only a little bit---and not at all if the map lookup was to stay in for now.)

[XFactHD]

2. Have getBurnOdds(state, level, pos) and getIgniteOdds(state, level, pos) check this.burnOdds/igniteOdds first (edit: then the data map) and only call the blockstate methods if there's no entry.

Hard disagree, the in-code hooks on the block-to-be-ignited must be able to have final say. Say I have a block that specifies certain burn and ignite odds in the datamap for the generic case and for use cases outside of a placed fire block interacting with the target block but then overrides the block extension methods to deny flammability under certain conditions. This would not be possible if the datamap takes precedence. Practical example: If not disabled in the config, all blocks in FramedBlocks burn by default unless the applied camo is itself not flammable. It's also worth noting that no other use of datamaps in parallel with in-code hooks gives precedence to the datamap, the hook for dynamic use cases always wins.