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

Refactor Better ChatPrint and fix size limit #19

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

Conversation

Derpius
Copy link
Member

@Derpius Derpius commented Dec 22, 2023

Changes

  • Adds a new TASUtils.getChatPrintMessageSizeBits global to calculate the size of a chat print message before trying to send
  • Refactors encodeMsg to have a clearer name and to not check the message size mid-send
  • Updates Broadcast and ChatPrint functions to check the message size and error before starting the netmsg
  • Replaces net.WriteColor with net.WriteUInt
    • WriteColor uses WriteUInt under the hood, but this way we ignore some useless checks and have confidence in parity between our writer and size calculator functions
  • Reduces max message size to 4KB

Impact

  • Properly prevents overflowing net messages
  • Exposes the size calculation func which allows for upcoming plycore changes to throw E2 errors when the message is too large

Testing

TASUtils.Broadcast and Player:ChatPrint() are essentially the same code, so can fully test one and then smoke test the other

  • Calling with no arguments prints an empty string to chat
  • Calling with one or more strings prints those strings to chat
  • Calling with colours and strings correctly colours the chat.AddText message
  • Arguments that are neither colours nor strings are stringified
  • Throws an error when the total message is larger than 4KB
  • Passing more than 255 arguments truncates to 255

Copy link
Contributor

@yogwoggf yogwoggf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Will do a QA soon

@yogwoggf
Copy link
Contributor

yogwoggf commented Feb 13, 2024

QA Result: Failed ❌

  • Calling with no arguments prints an empty string to chat ✅
  • Calling with one or more strings prints those strings to chat ✅
  • Calling with colours and strings correctly colours the chat.AddText message ✅
  • Arguments that are neither colours nor strings are stringified✅
  • Throws an error when the total message is larger than 4KB ✅
  • Passing more than 255 arguments truncates to 255 ❌
    • Passing more than 255 args doesn't truncate, but instead drop the entire message completely, leaving a blank line in the chatbox.
    • image
    • For clarity, the 256 message didn't throw an error, it silently failed.

Copy link
Contributor

@yogwoggf yogwoggf left a comment

Choose a reason for hiding this comment

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

Forgot to approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants