Skip to content

Conversation

@dries-c
Copy link
Member

@dries-c dries-c commented Dec 9, 2025

No description provided.

@dries-c dries-c requested a review from a team as a code owner December 9, 2025 21:27
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 9, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 9, 2025
if($this->outputType === self::TYPE_DATA_SET){
CommonTypes::putString($out, $this->unknownString);
}
CommonTypes::writeOptional($out, $this->unknownString, CommonTypes::putString(...));
Copy link
Member

Choose a reason for hiding this comment

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

Does the spec say what this is now?

Copy link

Choose a reason for hiding this comment

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

self::TYPE_JSON_WHISPER,
self::TYPE_JSON_ANNOUNCEMENT,
self::TYPE_JSON,
], true)){
Copy link
Member

@dktapps dktapps Dec 9, 2025

Choose a reason for hiding this comment

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

if(match($this->type){ self::TYPE_RAW => true }) would be better for this

self::TYPE_CHAT,
self::TYPE_WHISPER,
self::TYPE_ANNOUNCEMENT,
], true)){
Copy link
Member

Choose a reason for hiding this comment

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

Same again here

CommonTypes::getString($in); // systemMessage
CommonTypes::getString($in); // textObjectWhisper
CommonTypes::getString($in); // textObjectAnnouncement
CommonTypes::getString($in); // textObject
Copy link
Member

Choose a reason for hiding this comment

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

These should be validated so we don't have encoding symmetry inconsistencies. By which I mean, bail if the strings are not exactly what's expected

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how you could like to see it, every implementation i come up with is quite ugly

Copy link
Member

Choose a reason for hiding this comment

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

Probably a helper function that accepts the expected string would be fine

This comment was marked as off-topic.

self::TYPE_JSON_ANNOUNCEMENT,
self::TYPE_JSON,
], true)){
Byte::writeUnsigned($out, self::CATEGORY_MESSAGE_ONLY);
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this could be a problem for encoding symmetry too. The decoder doesn't validate that this matches the expected type

Copy link
Member Author

Choose a reason for hiding this comment

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

If we validate the decoding of the strings, it'll be symmetrical already.

Copy link
Member

Choose a reason for hiding this comment

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

The real type is decoded after this. So if we see a different category for TYPE_JSON e.g. it won't be re-encoded as we received it. So we should bail on decode if the category doesn't match the type.

pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 10, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 10, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 11, 2025
@WenoxGB
Copy link

WenoxGB commented Dec 13, 2025

there is any changes in PlayerSkinPacket?? bec when i do
$minion->setSkin($minionSkin);
$minion->sendSkin();
my entity skin not change after 1.21.130....

@DaisukeDaisuke
Copy link

@WenoxGB Could you please confirm that Only Allow Trusted Skins is turned off?
It seems that the item may be turned on again in some environments

image

@WenoxGB
Copy link

WenoxGB commented Dec 13, 2025

@DaisukeDaisuke yes its turned off..

when i reload the world, entity reconstruct than the skin show..

pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 14, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 14, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 14, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 16, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 16, 2025
we don't want to break packet encode/decode symmetry testability.
Putting it in the static constructor should avoid most side effects.
It's not ideal, but the alternative is forcing every place that might
have given empty strings to check manually, which would take a lot
more work.
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 16, 2025
@dktapps dktapps merged commit 62fcc59 into master Dec 16, 2025
13 checks passed
@dktapps dktapps deleted the bedrock-1.21.130 branch December 16, 2025 21:42
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.

6 participants