Skip to content

Fix showoff item cleanup on chunk unload and double-chest handling#64

Merged
MLG-SERBUR merged 1 commit into
masterfrom
codex/review-pull-request-#34-and-implement-changes
Apr 20, 2026
Merged

Fix showoff item cleanup on chunk unload and double-chest handling#64
MLG-SERBUR merged 1 commit into
masterfrom
codex/review-pull-request-#34-and-implement-changes

Conversation

@MLG-SERBUR
Copy link
Copy Markdown
Member

@MLG-SERBUR MLG-SERBUR commented Apr 20, 2026

Motivation

This PR started from a review request and then narrowed to only changes that are actually useful.

User prompt:
“review #34

tell me what it changes, and if each change is actually useful/needed or not.

then implement the definitely useful ones.”

Assistant response (summary):
Reviewed PR #34, split changes into useful vs unnecessary/risky, and implemented only the clearly useful fixes.


Conversation Summary (Prompt → Response)

User prompt:
“if the shop has to be a chest or doublechest, and the stock has to fit in that chest or doublechest, how is it possible for it to exceed the 54 slots?”

Assistant response (summary):
Agreed this cap is not needed in practice for chest/double-chest constrained stock, and reverted that clamp change.


User prompt:
“as for location#getBlock in getContainer - check the git history/blame to see if that's intentional. I have a feeling it might've been to ensure the blockstate is loaded and it's not pulling from cache?”

Assistant response (summary):
Checked history/blame and restored location.getBlock(); instead of removing it casually. Also investigated upstream API/server sources; evidence suggests it may be a legacy/defensive call rather than strictly required, but keeping it is safer without a focused refactor.


User prompt:
“for the despawn items - what if the placed block is not a shop? ... wouldn't this miss shops that are turned into doublechests with a non-shop chest block?”

Assistant response (summary):
Investigated shop creation flow and double-chest resolution. Current logic using shopAPI.isShop(container) should still detect merged double chests when one side is already a shop, while avoiding non-shop cleanup.


User prompt:
“print the commit where location#getblock was introduced. Also check if my prior question was correct or not...”

Assistant response (summary):
Identified introduction commit as f75f7058d8591b6d7b55179900867e97325054e0 and confirmed rationale analysis above.


Final Scope in This PR

Kept (definitely useful)

  • Fix ShowoffItem chunk-unload matching to compare world + chunk coordinates instead of chunk object identity.
  • Fix double-chest despawn to remove both left and right showcase positions (instead of despawning left twice).
  • Keep shop check gate so cleanup only runs for recognized shops.

Reverted / Not Included

  • Removed 54-slot post-purchase inventory clamp (not needed under chest/double-chest stock constraints).
  • Kept location.getBlock(); in ShopAPI#getContainer (restored after history/blame review).

@MLG-SERBUR MLG-SERBUR marked this pull request as ready for review April 20, 2026 02:40
@MLG-SERBUR MLG-SERBUR merged commit 2725679 into master Apr 20, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant