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

implement hoppers #884

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

implement hoppers #884

wants to merge 28 commits into from

Conversation

RestartFU
Copy link
Contributor

@RestartFU RestartFU commented May 29, 2024

No description provided.

@RestartFU RestartFU marked this pull request as ready for review May 29, 2024 04:04
@RestartFU

This comment was marked as outdated.

@RestartFU
Copy link
Contributor Author

should be good for review, fully working now

@DaPigGuy DaPigGuy mentioned this pull request May 31, 2024
89 tasks
// Hopper is a model used by hoppers.
type Hopper struct{}

// BBox returns a physics.BBox that spans a full block.
Copy link
Member

Choose a reason for hiding this comment

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

The hopper model is not a full block. There is a small hole at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't seem to find the model bbox, would you have it?

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +70 to +85
w := e.World()
pos := cube.PosFromVec3(e.Position())
blockPos := pos.Side(cube.FaceDown)

bl, ok := w.Block(blockPos).(block.Hopper)
if ok && !bl.Powered && bl.CollectCooldown <= 0 {
_, err := bl.Inventory().AddItem(i.i)
if err != nil {
// We couldn't add any of the item to the inventory, so we ignore it.
return i.passive.Tick(e)
}

_ = e.Close()
bl.CollectCooldown = 4
w.SetBlock(blockPos, bl, nil)
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be good if this could be moved to the actual Hopper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not possible due to import cycles

server/block/hopper.go Outdated Show resolved Hide resolved
server/block/hopper.go Outdated Show resolved Hide resolved

// Facing is the direction the hopper is facing.
Facing cube.Face
// Powered is whether the hopper is powered or not.
Copy link
Member

Choose a reason for hiding this comment

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

Add specifics on what powered vs. not powered means for the hopper

h.CollectCooldown--
h.LastTick = currentTick

if h.TransferCooldown > 0 || h.CollectCooldown >= 0 {
Copy link
Member

@DaPigGuy DaPigGuy Jun 5, 2024

Choose a reason for hiding this comment

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

Why are the operators different between these two conditions? Shouldn't the second condition be h.CollectCooldown > 0?

Copy link
Member

Choose a reason for hiding this comment

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

And should transfer cooldown be blocking collecting, and vice versa?


// HopperInsertable represents a block that can have its contents inserted into by a hopper.
type HopperInsertable interface {
Container
Copy link
Member

Choose a reason for hiding this comment

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

The block does not necessarily have to implement the Container interface, e.g. jukeboxes and composters

Copy link
Member

Choose a reason for hiding this comment

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

Also please implement behavior for those two blocks 😅

Container

// InsertItem attempts to insert a single item into the container. If the insertion was successful, the item is
// returned. If the insertion was unsuccessful, the item stack returned will be empty. InsertItem by itself does
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems to be outdated, there is no item stack involved in the return type.


// insertItem inserts an item into a container from the hopper.
func (h Hopper) insertItem(pos cube.Pos, w *world.World) bool {
dest, ok := w.Block(pos.Side(h.Facing)).(Container)
Copy link
Member

Choose a reason for hiding this comment

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

This insert item logic works under the assumption that there is an inventory associated with the block, which is a big no, see comment above.

This will need some refactoring so I haven't reviewed this in detail yet.


// HopperExtractable represents a block that can have its contents extracted by a hopper.
type HopperExtractable interface {
Container
Copy link
Member

Choose a reason for hiding this comment

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

Same problem with HopperInsertable, have not reviewed in detail.

RestartFU and others added 2 commits June 4, 2024 23:57
Co-authored-by: DaPigGuy <mcpepig123@gmail.com>
Co-authored-by: DaPigGuy <mcpepig123@gmail.com>
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.

None yet

2 participants