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

Fix for https://github.com/Creators-of-Create/Create/issues/7250 #7829

Open
wants to merge 4 commits into
base: mc1.20.1/dev
Choose a base branch
from

Conversation

dan96kid
Copy link

@dan96kid dan96kid commented Mar 8, 2025

"Fine, I'll do it myself." - Thanos

This is a fix for #7250 that takes text on the front and back of signs into account

dan96kid added 2 commits March 7, 2025 16:06
Fix for Creators-of-Create#7250 that should hopefully not break anything
Fix for Creators-of-Create#7250 that should hopefully not break anything. Takes the text on the front and back of signs into account.
@VoidLeech VoidLeech added the pr type: fix PR fixes a bug label Mar 8, 2025
for (int i = 0; i < text.size() && i + line < 4; i++) {
if (i == 0)
reserve(i + line, sign, context);
if (i > 0 && isReserved(i + line, sign, context))
break;

signText = signText.setMessage(i + line, text.get(i));
//signText = signText.setMessage(i + line, text.get(i));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can just be removed.

Copy link
Author

Choose a reason for hiding this comment

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

I agree.

sign.setText(signText, side);
context.level()
.sendBlockUpdated(context.getTargetPos(), sign.getBlockState(), sign.getBlockState(), 2);
context.level().sendBlockUpdated(context.getTargetPos(), sign.getBlockState(), sign.getBlockState(), 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should set the text before telling the level the block got updated.

Copy link
Author

Choose a reason for hiding this comment

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

Okay.

I am now wondering... Do both sides of the sign need their own sendBlockUpdate(), or can we get away with only one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One is enough. The previous code only called it once. And please wrap the part of the changes after if (changed) into {}

@VoidLeech VoidLeech added the pr flag: simple PR has minimal changes label Mar 8, 2025
dan96kid added 2 commits March 8, 2025 11:07
- Removed unused import statement.
- Wrapped all code in the if statements with {}.
- Removed a seemingly unnecessary `sendBlockUpdated`.
Comment on lines +23 to +24
SignText signTextFront = ((SignBlockEntity) be).getFrontText();
SignText signTextBack = ((SignBlockEntity) be).getBackText();

Choose a reason for hiding this comment

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

Wouldn't that be better?

Suggested change
SignText signTextFront = ((SignBlockEntity) be).getFrontText();
SignText signTextBack = ((SignBlockEntity) be).getBackText();
SignText signTextFront = sign.getFrontText();
SignText signTextBack = sign.getBackText();

Choose a reason for hiding this comment

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

Also you don't need {} in every if statement, just in the if (changed) { ... }

@NickTheEASDude
Copy link

Does this also fix the issue where the sign's data was completely cleared when the text got updated?

@dan96kid
Copy link
Author

Does this also fix the issue where the sign's data was completely cleared when the text got updated?

As far as I can tell, yes. This fix preserves both the text on the sign and any dyes that were applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr flag: simple PR has minimal changes pr type: fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants