Skip to content

Fix dropped items looking like 1 item with item sanitation#12260

Closed
Warriorrrr wants to merge 1 commit into
PaperMC:mainfrom
Warriorrrr:fix/item-sanitize-count-clump
Closed

Fix dropped items looking like 1 item with item sanitation#12260
Warriorrrr wants to merge 1 commit into
PaperMC:mainfrom
Warriorrrr:fix/item-sanitize-count-clump

Conversation

@Warriorrrr
Copy link
Copy Markdown
Member

Closes #12252

This seems like the most readable approach to this, rather than cooking up some expression to do this in one line

@Warriorrrr Warriorrrr added the type: bug Something doesn't work as it was intended to. label Mar 10, 2025
@Warriorrrr Warriorrrr requested a review from a team as a code owner March 10, 2025 19:36
@github-project-automation github-project-automation Bot moved this to Awaiting review in Paper PR Queue Mar 10, 2025
@Owen1212055
Copy link
Copy Markdown
Member

Owen1212055 commented Mar 10, 2025

Yeah so, this technically works. Has the consequence of causing ALL items to no longer have count "sanitized" (just converted like this), however.
Imo a more proper solution would be to add something to the syncher to denote that it shouldn't be sanitized.

if (GlobalConfiguration.get().anticheat.obfuscation.items.binding.getAssetObfuscation(itemStack).sanitizeCount()) {
return 1;
} else {
if (!GlobalConfiguration.get().anticheat.obfuscation.items.binding.getAssetObfuscation(itemStack).sanitizeCount() || count <= 1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe first check count? Seems like a free performance gain for most equipment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In reality, it most likely does not matter that much and honestly, branch prediction taking the wrong path might be worse.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's indeed not worth big discussion as the gain is minimal, so won't continue past this message explaining my thoughts. If someone could mark this as resolved would be glad

Since the check is already present, it makes sense to make the easy check first that already has all the data on stack instead of the pointer chasing through all the ram, especially since in current implementation the majority of items passing here will be from entiy equipment, which is mostly either 1 for unstackable items (armor) or 0 for empty slots, so since it's true it's not gonna even need to check the config

@lynxplay
Copy link
Copy Markdown
Contributor

I wonder if it wouldn't be smarted to replace the codecs used by the entity data accessors that we actually want to sanitize and do the session starting there.

@OfficialTheosis
Copy link
Copy Markdown

OfficialTheosis commented Mar 20, 2025

I added a comment with additional info to #12252 (#12252 (comment)), so I don't think this entirely closes it anymore.

@Warriorrrr
Copy link
Copy Markdown
Member Author

That probably should've been in a separate issue, but I'll close this for now anyways

@Warriorrrr Warriorrrr closed this Mar 20, 2025
@github-project-automation github-project-automation Bot moved this from Awaiting review to Closed in Paper PR Queue Mar 20, 2025
@OfficialTheosis
Copy link
Copy Markdown

That probably should've been in a separate issue, but I'll close this for now anyways

Do you want me create a separate issue for it now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug Something doesn't work as it was intended to.

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

Item obfuscation affects item entities' quantity + firework rocket explosions

6 participants