Skip to content
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

[21.1+] Generalise lightable blocks #1186

Conversation

Spinoscythe
Copy link
Contributor

What the title says
Fixed #1179

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@Spinoscythe Spinoscythe marked this pull request as ready for review June 25, 2024 20:30
@Matyrobbrt Matyrobbrt added enhancement New (or improvement to existing) feature or request 1.21 Targeted at Minecraft 1.21 labels Jun 25, 2024
Copy link
Member

@embeddedt embeddedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SpecialLightableBlock interface is not actually implemented on any of the blocks. Additionally, is setLit really required? Seems simple enough to require custom blocks to at least use the common BlockStateProperties.LIT property.

serverlevel.setBlockAndUpdate(blockpos, BaseFireBlock.getState(serverlevel, blockpos));
serverlevel.gameEvent(null, GameEvent.BLOCK_PLACE, blockpos);
- } else if (CampfireBlock.canLight(blockstate) || CandleBlock.canLight(blockstate) || CandleCakeBlock.canLight(blockstate)) {
+ } else if (blockstate.getBlock() instanceof net.neoforged.neoforge.common.SpecialLightableBlock && ((net.neoforged.neoforge.common.SpecialLightableBlock) blockstate.getBlock()).canLight(blockstate)) {
serverlevel.setBlockAndUpdate(blockpos, blockstate.setValue(BlockStateProperties.LIT, Boolean.valueOf(true)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still sets the LIT property here

BlockState blockstate = level.getBlockState(blockpos);
boolean flag = false;
- if (!CampfireBlock.canLight(blockstate) && !CandleBlock.canLight(blockstate) && !CandleCakeBlock.canLight(blockstate)) {
+ if (blockstate.getBlock() instanceof net.neoforged.neoforge.common.SpecialLightableBlock && !((net.neoforged.neoforge.common.SpecialLightableBlock) blockstate.getBlock()).canLight(blockstate)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong logic, instead of catching unlightable blocks and lightable blocks that refuse, it only catches the latter. Should be !(instanceof) || !canLight()

I don't see the else, but I suspect it also sets LIT

@HenryLoenwind
Copy link
Contributor

The SpecialLightableBlock interface is not actually implemented on any of the blocks. Additionally, is setLit really required? Seems simple enough to require custom blocks to at least use the common BlockStateProperties.LIT property.

It may be a machine or a multiblock formation trigger or something that triggers an action but doesn't actually need a blockstate for that.

@HenryLoenwind
Copy link
Contributor

After looking around a bit, this seems more like a candidate for ItemAbilities. Sorry, I hadn't seen those before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Targeted at Minecraft 1.21 enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] Lightable blocks generalisation
4 participants