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

Correct Interactive Block Behavior #388

Merged
merged 9 commits into from
Dec 27, 2024

Conversation

OfficialKris
Copy link
Contributor

@OfficialKris OfficialKris commented Dec 9, 2024

Description

Fixes #275 and many other interactive block issues:

  • All interactive blocks are per block now (for instance, chests and furnaces do not share containers globally)
  • Crafting table containers are both unique per player AND per block
  • Breaking interactive blocks removes all contents (can drop items once item drop is implemented)
  • All active container users are forcibly closed when another player removes a block
  • Removed artificial cap on container screen limit which kicked players after opening a container 15 times (we may not be handling container ids correctly)
  • Added a standard set of container functions to reduce code duplication for interactive blocks

This PR requires #384

Testing

Two clients

  • Place a crafting table, a furnace, and a chest
  • Open each and place items in container
  • Open crafting table again, notice no items are stored
  • Open all other blocks and see items in both clients
  • Open crafting table in both and notice containers are not shared

Internally, this change maintains the OpenContainer HashMap and a new AtomicU32 which tracks the current index for the generation of uuids. On large persistent servers, unique containers could be optimized to reduce memory usage. Every user who opens a unique container will generate and cache a new data structure (think 10,000 users opening a crafting table, enderchest, etc). Future work could include separating unique containers (crafting table) and persistent unique containers (ender chest) into separate data structures (ex. stacks instead of maps).

Also, another optimization could be made to client interactive block interactions. Currently, this requires that the server iterate through the entirety of the OpenContainer HashMap. It may be better to create a sort of registry which maps block position to OpenContainerID in the long term (again large server optimizations). Otherwise, the algorithm is randomized brute force with a time complexity of O(n + m).

Checklist

Things need to be done before this Pull Request can be merged.

  • Code is well-formatted and adheres to project style guidelines: cargo fmt
  • Code does not produce any clippy warnings: cargo clippy
  • All unit tests pass: cargo test
  • I added new unit tests, so other people don't accidentally break my code by changing other parts of the codebase. How?

}
}
drop(open_containers);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what's the need for manually managing memory as if it was C

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what's the need for manually managing memory as if it was C

I think it's not necassary here but it's more like freeing the guard then managing the memory.
For example if you call an sub function that trys to lock the variable too you create an deadlock.
Which results in freezing

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 think this is a draft and in progress :D I think I was trying to fix a different issue that I thought was from a deadlock. I will probably refactor this code

@OfficialKris OfficialKris marked this pull request as ready for review December 24, 2024 20:58
@Snowiiii
Copy link
Member

When opening a Chest, the server freezes and i get a timeout

@OfficialKris
Copy link
Contributor Author

Fixed, tried to optimize code like this was C.

@Snowiiii
Copy link
Member

Okay works now, But i found a bug. If you shift click a item from a chest into your inventory and reopen the chest the item will be in the chest again

@OfficialKris
Copy link
Contributor Author

I checked comparing with master and it seems shift clicking is just not supported yet. Shift clicking into containers does the same thing. I believe that should be a separate PR since it would be modifying other interaction container code.

@Snowiiii
Copy link
Member

Waiting for @Bryntet review, but for some reason crafting does not work anymore :c

@Bryntet
Copy link
Contributor

Bryntet commented Dec 26, 2024

@Snowiiii IIRC crafting is currently broken on master too? or am I misremembering

@Snowiiii
Copy link
Member

@Snowiiii IIRC crafting is currently broken on master too? or am I misremembering

yep, its broken on master

@Snowiiii Snowiiii merged commit c521ef8 into Pumpkin-MC:master Dec 27, 2024
9 checks passed
@Snowiiii
Copy link
Member

Merged! Thanks for your hard work on this, @OfficialKris ❤️

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.

Items placed in the crafting table are shared globaly
5 participants