-
-
Notifications
You must be signed in to change notification settings - Fork 175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Changes to fire and block interactions. #1469
Comments
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 |
And there's more... Regarding the implementation of getFireSpreadSpeed() and getFlammability(), I see a couple of issues:
Suggestion:
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. |
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. |
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.) |
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. |
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
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:
FireBlock.checkBurnOut()
(see below)FireBlock.checkBurnOut()
that is posted after onReplaceByFire() with the same semantics.DispenserBlock.registerBehavior(Items.FLINT_AND_STEEL
gets no replacement, it already is handled by the ItemAbility.The text was updated successfully, but these errors were encountered: