neoforged / NeoForge

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

[Request] Lightable blocks generalisation #1179

Open HenryLoenwind opened 1 week ago

HenryLoenwind commented 1 week ago

FlintAndSteelItem has hardcoded support for 3 types of blocks it can light.

if (!CampfireBlock.canLight(blockstate) && !CandleBlock.canLight(blockstate) && !CandleCakeBlock.canLight(blockstate)) {
...
level.setBlock(blockpos, blockstate.setValue(BlockStateProperties.LIT, Boolean.valueOf(true)), 11);

While it is easy to make a block that can be lit by "flint and steel", and it also is easy to make an item that can light camp fires, candles and cakes, making them in a way they can interact (i.e. having a modded lightable block and a modded lighter from another mod) isn't that easy.

Suggestion:

interface ILightableBlock {
  default boolean canLight(BlockState pState) {
    return CampfireBlock.canLight(blockstate) || CandleBlock.canLight(blockstate) || CandleCakeBlock.canLight(blockstate);
  }
  void setLit(LevelAccessor pLevel, BlockState pState, BlockPos pPos, boolean pLit);
}

This matches the methods CandleBlock already has. I haven't checked the other two. If they are too different, the interface should use a default method to dispatch that one, too.

For the Flint and Steel, the patch would be something like

-        if (!CampfireBlock.canLight(blockstate) && !CandleBlock.canLight(blockstate) && !CandleCakeBlock.canLight(blockstate)) {
+        if (!(blockstate.getBlock() instanceof ILightableBlock) || !((ILightableBlock)blockstate.getBlock()).canLight(blockstate)) {
-        } else {
+        } else if (blockstate.getBlock() instanceof ILightableBlock ilb) {
-            level.setBlock(blockpos, blockstate.setValue(BlockStateProperties.LIT, Boolean.valueOf(true)), 11);
+            ilb.setLit(level, blockstate, blockpos, true);

This could be solved the other way around, but I think firestarting items should be the main driver in this interaction as modded ones may have the biggest difference from how flint and steel works. Including setLit() instead of keeping the hardcoded LIT property gives blocks enough control over the process.

(This code suggestion is, as far as it may be protected by copyright laws, licensed PD (US and other compatible countries) or CC0 (EU etc.).)

Soaryn commented 1 week ago

Might actually be worth looking into making this a BlockCapability instead of a block implementing an interface directly.

HenryLoenwind commented 1 week ago

K.I.S.S.

I really doubt there'd be enough advantages to outweigh the extra effort needed to implement this. For both nf and the modded blocks that'd implement it. And on second thought, slapping it onto IBlockExtension would an even simpler solution. There's no reason not to have every block reject being lit...

KnightMiner commented 4 days ago

I feel like setLit should have a default implementation that sets the lit property, given its used by all 3 vanilla blocks that work like that. Would also make it easier to patch the interface into the relevant vanilla blocks.

HenryLoenwind commented 4 days ago

I feel like setLit should have a default implementation that sets the lit property, given its used by all 3 vanilla blocks that work like that. Would also make it easier to patch the interface into the relevant vanilla blocks.

As I commented on the now closed PR, this feels like it would work well as a ToolAction. "Right-click leading to changed blockstate" is exactly what those that get patched into mc do.

sciwhiz12 commented 14 hours ago

Instead of a separate interface, why not a method on the Block extension interface? Both suggested interface methods could even be rolled into one method, which does the action if possible and returns a boolean on whether it did so (for continuing the interaction chain if it cannot be lit).

HenryLoenwind commented 11 hours ago

Mostly because IBlockExtension wasn't on my radar when I wrote this, but the older single-use interfaces were.

Now that I have my NF dev environment set up, I could PR this myself. The question is only whether this would work better as an ItemAbility or a stand-alone block extension. It is awfully close to what shears and axes do with getToolModifiedState().