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

update tablist on heart blink and username style change #99

Merged
merged 5 commits into from
Aug 30, 2023
Merged

update tablist on heart blink and username style change #99

merged 5 commits into from
Aug 30, 2023

Conversation

JustAlittleWolf
Copy link
Contributor

Fixes the issue where the hearts get stuck blinking when showing the health scoreboard in the tablist also fixes updates now when the style of playerInfo.getTabListDisplayName() changes

Fixes the issue where the hearts get stuck blinking when showing the health scoreboard in the tablist
also fixes updates now when the style of playerInfo.getTabListDisplayName() changes
@JustAlittleWolf JustAlittleWolf changed the title render on heart blink and username style change update tablist on heart blink and username style change Aug 27, 2023
@JustAlittleWolf
Copy link
Contributor Author

huh why does it fail?

@tr7zw
Copy link
Owner

tr7zw commented Aug 27, 2023

Seems to be missing permissions to attach the junit report.

@andreasdc
Copy link

Good job guys, Exordium is one of the best mods that exists. ❤️

if (playerInfo.getTabListDisplayName() != null) {
combinedHashes += playerInfo.getTabListDisplayName().getString().hashCode();
Style displaynameStyle = playerInfo.getTabListDisplayName().getStyle();
combinedHashes += (displaynameStyle.getColor() == null ? 0 : displaynameStyle.getColor().getValue()) + (displaynameStyle.isBold() ? 1 : 0) + (displaynameStyle.isItalic() ? 3 : 0) + (displaynameStyle.isObfuscated() ? 7 : 0) + (displaynameStyle.isUnderlined() ? 15 : 0) + (displaynameStyle.isStrikethrough() ? 31 : 0);
Copy link
Owner

Choose a reason for hiding this comment

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

I think there are better ways to do this. Shouldn't the playerInfo.hashCode() do the trick and skip this entire block? (Assuming it's implemented correctly by Mojang)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be cleaner code, but I am not sure how the defaut java hashCode method works. Mojang themselves haven't implemented a hashCode function for the PlayerInfo Object so I think it just takes the hashcodes of all of its fields and does some math to combine them into one integer. This is a bit slower than just taking the few specific fields that change how the displayname looks in the tablist and calculating the hashcode from them.

Copy link
Owner

Choose a reason for hiding this comment

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

Hashcode always has to be somewhat fast. Good chance this entire line with its conditional checks is slower than just getting the hashcode of the entire thing. For the sake of a bit more manageable code I'd give it a shot with just combinedHashes += playerInfo.hashCode() and see if that works as expected. I think I'll also soon add a delay timer to how often it checks the update condition. At 1000 fps with no changes going on it checks all the things 1000 times per second. That's faster than rendering it 1000 times per second, but still a waste of time. 60 or maybe 20 times per second for some should already do the trick. This would probably also explain some of the performance loss compared to the 1.19.3 version. With this fix, the time for the hashcode calculation is next to 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I will fix it but probably only tomorrow, thanks for pointing this out ^^

Copy link
Owner

Choose a reason for hiding this comment

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

Yea no worries/hurry. Already helping a lot with this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, so I just tested it and replacing the block with playerInfo.hashCode() does not yield the same results as the previous code did. To be specific it only seems to be missing an update check on for the player skin location, which causes the tablist to have default skins until some other factor is updated.
image
looks like this for a while.
Another way to make the code cleaner would be to move the playerInfo hashing to a seperate method, or simply add the skin location hash to combinedHashes. I am also not sure if it properly updates on a latency change, although that is hard to test in my current environment.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah sorry I had the wrong variable. What I meant was using:

if (playerInfo.getTabListDisplayName() != null) {
combinedHashes += playerInfo.getTabListDisplayName().hashCode();
}

without breaking it up into checking the color, style, name, bold, strikethrough etc. while keeping all the rest like

combinedHashes += playerInfo.getSkinLocation().hashCode();
combinedHashes += playerInfo.getLatency() * 63;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see that does make a lot more sense. Mojang does have a hashCode method for the Style object so this should work properly ^^
Fixed with bb864a8

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good. Will test it later, rn need to do some irl stuff. Maybe I'll also implement the poll rate limiter later today.

@tr7zw tr7zw merged commit c5a824a into tr7zw:1.20 Aug 30, 2023
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.

3 participants