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

Remove invalid sign check #3328

Merged
merged 3 commits into from
Apr 28, 2024
Merged

Remove invalid sign check #3328

merged 3 commits into from
Apr 28, 2024

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Mar 12, 2024

... or add a way to skip this check, because for some characters like | this is simply not correct (e.g. you can have 45 bars on the line): |||||||||||||||||||||||||||||||||||||||||||||

@extremeheat
Copy link
Member

Many of these checks are designed to prevent the bot from getting flagged/kicked by the server. Have you checked how this is handled my vanilla?

@zardoy
Copy link
Contributor Author

zardoy commented Mar 14, 2024

Many of these checks are designed to prevent the bot from getting flagged/kicked by the server. Have you checked how this is handled my vanilla?

Of course, it does handle normally. Actually, I didn't see any restrictions on the line length of the sign and its not stated in the protocol docs either. In the vanilla, the sign editor calculates the width of each character to get the maximum length of the line. For example one | ' , . ; character is equal to 1 / 3 of the a character. Or ( ) [ ] { } and space is equal to 1 / 1.46 of regular character and so on. So, the maximum length of the line is still 15 characters but its only if you take into account the specific width of these characters

@rom1504 rom1504 added this to Needs triage in PR Triage Mar 17, 2024
@rom1504
Copy link
Member

rom1504 commented Mar 17, 2024

did you check this for minecraft 1.8 ?

@rom1504 rom1504 moved this from Needs triage to Waiting for user input in PR Triage Mar 17, 2024
@zardoy
Copy link
Contributor Author

zardoy commented Mar 17, 2024

@rom1504 Of course! It does work without any issues. The maximum length you can get is 45 characters on the line

@zardoy
Copy link
Contributor Author

zardoy commented Mar 17, 2024

GitHub Action says:

1 failing
1) mineflayer_external 1.20.4v
     sign:
   Uncaught TypeError: Cannot read properties of undefined (reading 'trimEnd')
    at Timeout._onTimeout (test/externalTests/sign.js:23:42)
    at listOnTimeout (node:internal/timers:569:17)
    at process.processTimers (node:internal/timers:512:7)

The 400k lines log is crazy, but i don't think its current PR breaks the pipeline

@rom1504
Copy link
Member

rom1504 commented Mar 18, 2024

If the max is 45 characters then why not adapt the check instead of removing it ?

@zardoy
Copy link
Contributor Author

zardoy commented Mar 18, 2024

If the max is 45 characters then why not adapt the check instead of removing it ?

Okay, not a problem.


As said in #3328 (comment) In the original client, if you type only a the maximum length is 15, if you type only . the maximum length is 45, but again, nothing bad happens if you send 20 a to the vanilla server

@zardoy
Copy link
Contributor Author

zardoy commented Mar 23, 2024

Is this good to go?

@zardoy
Copy link
Contributor Author

zardoy commented Apr 16, 2024

@extremeheat could you look into this as well? Don't see anything that blocks the pr

@rom1504 rom1504 merged commit ec76468 into PrismarineJS:master Apr 28, 2024
22 checks passed
PR Triage automation moved this from Waiting for user input to Closed Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PR Triage
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants