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

Brewing Stands #916

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Brewing Stands #916

wants to merge 19 commits into from

Conversation

xNatsuri
Copy link
Contributor

@xNatsuri xNatsuri commented Sep 1, 2024

This PR implements brewing stands

Built off #632
Resolves #520

server/conf.go Outdated Show resolved Hide resolved
Copy link
Member

@TwistedAsylumMC TwistedAsylumMC left a comment

Choose a reason for hiding this comment

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

Just a few things, staticcheck seems to also be kicking up a fuss about some things but not really sure what's up with that

server/item/recipe/vanilla.go Outdated Show resolved Hide resolved
server/session/player.go Outdated Show resolved Hide resolved
// all these imports. This ensures that all items are registered before the creative items are registered
// in the init function in this package.
// The following four imports are essential for this package: They make sure this package is loaded after
// all these imports. This ensures that all blocks anditems are registered before the creative items are
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// all these imports. This ensures that all blocks anditems are registered before the creative items are
// all these imports. This ensures that all blocks and items are registered before the creative items are

Comment on lines +42 to +46

// LightEmissionLevel ...
func (b BrewingStand) LightEmissionLevel() uint8 {
return 1
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// LightEmissionLevel ...
func (b BrewingStand) LightEmissionLevel() uint8 {
return 1
}

Brewing stands don't emit light on Bedrock

// BreakInfo ...
func (b BrewingStand) BreakInfo() BreakInfo {
drops := b.inventory.Items()
drops = append(drops, item.NewStack(b, 1))
Copy link
Member

Choose a reason for hiding this comment

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

It may be better to drop the inventory items in a break handler, similar to chests/furnaces

if brewingSlot == 0 || brewingSlot == 4 || !brewingStack.Empty() {
continue
}
slot = brewingSlot
Copy link
Member

Choose a reason for hiding this comment

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

Missing validation here to make sure the item is usable in a brewing recipe

}

if ok || okTwo {
_, containerChange := recipe.(PotionContainerChange)
Copy link
Member

Choose a reason for hiding this comment

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

containerChange and ok are equivalent


if ok || okTwo {
_, containerChange := recipe.(PotionContainerChange)
inputItems2 := make([]world.Item, len(recipe.Input()))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this named inputItems2?

}

// ValidBrewingReagent checks if the world.Item is a brewing reagent.
func ValidBrewingReagent(itemName string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Why not take a world.Item here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement brewing stands
3 participants