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

Handle ClientboundTickingStatePacket correctly and fix Throwable Scales #4850

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

Conversation

letsgoawaydev
Copy link
Contributor

Handles custom tick rates sent from the ClientboundTickingStatePacket correctly. This freezes all tickable entitys and changes the geyser session tick() function update speed correctly.

Copy link
Member

@Konicai Konicai left a comment

Choose a reason for hiding this comment

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

thank you!

@Konicai
Copy link
Member

Konicai commented Jul 13, 2024

It would also be nice to implement ClientboundTickingStepPacket if you're able

@letsgoawaydev letsgoawaydev force-pushed the tick-rate-handling branch 2 times, most recently from 47e69a1 to 83fab59 Compare July 13, 2024 07:40
@letsgoawaydev
Copy link
Contributor Author

It would also be nice to implement ClientboundTickingStepPacket if you're able

Fixed in latest commit.

@letsgoawaydev letsgoawaydev changed the title Handle ClientboundTickingStatePacket correctly Handle ClientboundTickingStatePacket correctly and fix Throwable Scales Jul 13, 2024
@Konicai
Copy link
Member

Konicai commented Jul 14, 2024

Can review in 12hrs

@Konicai Konicai added the PR: Bugfix When a PR contains a bugfix label Jul 15, 2024
@letsgoawaydev
Copy link
Contributor Author

Some issues I've noticed, handling of block breaking speed when tick rate is less than 20 is not correct and player speed/gravity is not correct

@letsgoawaydev
Copy link
Contributor Author

letsgoawaydev commented Jul 30, 2024

Also animations are way too quick on the bedrock side, for other players I think we could send that they have mining fatigue to the session so that their arm animation speeds are slower

Most stuff like this we probably cant fix, though

@Camotoy
Copy link
Member

Camotoy commented Jul 30, 2024

Yeah, I don't think we should aim to resolve every single thing, seeing as Bedrock will still be operating under a 20 ticks per second cycle. I assume most people will be using this as a debugging tool, or is that assessment incorrect?

@letsgoawaydev
Copy link
Contributor Author

Yeah, I don't think we should aim to resolve every single thing, seeing as Bedrock will still be operating under a 20 ticks per second cycle. I assume most people will be using this as a debugging tool, or is that assessment incorrect?

Its mostly used for testing redstone, in my case I use it for testing plugins.
Also, I think that drawTick should be handled at 20 TPS because Bedrock still updates at 20 TPS, and the rendering system in Java updates every frame afaik. Using 20 tps would ensure that it doesnt update too much on fast tickrates while also similarly matching Java

@letsgoawaydev
Copy link
Contributor Author

Yeah, I don't think we should aim to resolve every single thing, seeing as Bedrock will still be operating under a 20 ticks per second cycle. I assume most people will be using this as a debugging tool, or is that assessment incorrect?

Its mostly used for testing redstone, in my case I use it for testing plugins.

Also, I think that drawTick should be handled at 20 TPS because Bedrock still updates at 20 TPS, and the rendering system in Java updates every frame afaik. Using 20 tps would ensure that it doesnt update too much on fast tickrates while also similarly matching Java

nvm i dont think it updates the tick count every frame, should keep draw tick updating at the same speed as the game

Copy link
Member

@Konicai Konicai left a comment

Choose a reason for hiding this comment

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

Also, the setInvisible method for throwable items used to be able to force the entity to be invisible. would be nice to restore that behaviour

@letsgoawaydev
Copy link
Contributor Author

I'm going to try complete this today, if i can't finish it today I will try tomorrow.

@@ -91,7 +92,8 @@ public static void sendCooldown(GeyserSession session) {
*/
private static void computeCooldown(GeyserSession session, CooldownType sessionPreference, long lastHitTime) {
if (session.isClosed()) return; // Don't run scheduled tasks if the client left
if (lastHitTime != session.getLastHitTime()) return; // Means another cooldown has started so there's no need to continue this one
if (lastHitTime != session.getLastHitTime())
return; // Means another cooldown has started so there's no need to continue this one
Copy link
Member

Choose a reason for hiding this comment

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

revert or add braces

@@ -58,7 +58,8 @@ public static void sendCooldown(GeyserSession session) {
CooldownType sessionPreference = session.getPreferencesCache().getCooldownPreference();
if (sessionPreference == CooldownType.DISABLED) return;

if (session.getAttackSpeed() == 0.0 || session.getAttackSpeed() > 20) return; // 0.0 usually happens on login and causes issues with visuals; anything above 20 means a plugin like OldCombatMechanics is being used
if (session.getAttackSpeed() == 0.0 || session.getAttackSpeed() > 20)
return; // 0.0 usually happens on login and causes issues with visuals; anything above 20 means a plugin like OldCombatMechanics is being used
Copy link
Member

Choose a reason for hiding this comment

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

revert or add braces

@letsgoawaydev
Copy link
Contributor Author

Okay, I've fixed title animations, attack indicator and some item cooldowns to be updated with tick rate. I'm not sure what other things require changes but it should be ready for testing if anyone can

@@ -652,6 +655,17 @@ public class GeyserSession implements GeyserConnection, GeyserCommandSource {

private MinecraftProtocol protocol;

@Getter
Copy link
Member

Choose a reason for hiding this comment

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

The class is annotated with a getter, this is likely unnecessary

@@ -652,6 +655,17 @@ public class GeyserSession implements GeyserConnection, GeyserCommandSource {

private MinecraftProtocol protocol;

@Getter
private int nanosecondsPerTick = 50000000;
@Getter
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bugfix When a PR contains a bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants