Skip to content

Conversation

@bmstefanski
Copy link

I've found that connecting to a server with minecraft version 1.20.4 didn't work - throwing quite ambiguous error An internal error occurred in your connection, but after trying different things for a while I figured it works well on 1.20 and only some servers throw that error. I compared the packet trace between those two versions and one difference I found was that in 1.20.4, servers send start_configuration packets during gameplay to reconfigure clients, but mineflayer's physics plugin was missing handlers for this. The bot continued sending position packets while in configuration state, which is invalid and causes the server to kick the client

Copy link
Contributor

@Omena0 Omena0 left a comment

Choose a reason for hiding this comment

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

LGTM

@rom1504
Copy link
Member

rom1504 commented Aug 21, 2025

Tests fail

I think we should rather disable a lot more than specifically physics during configuration phases

And it would be best to add a test for this (probably an internal test), so it will keep working

@Omena0
Copy link
Contributor

Omena0 commented Aug 22, 2025

Tests fail

I think we should rather disable a lot more than specifically physics during configuration phases

And it would be best to add a test for this (probably an internal test), so it will keep working

Those seem like flaky tests to me.

They don't have anything to do with this pr

@tcortega
Copy link

Could this be merged?

@rom1504
Copy link
Member

rom1504 commented Sep 16, 2025

No please see my previous message on what still needs to be done here

@tcortega
Copy link

No please see my previous message on what still needs to be done here

The tests that failed did not seem related at all to the PR, as Omena0 said above.

@tcortega
Copy link

No please see my previous message on what still needs to be done here

Could you please try re-running the tests that failed?

@rom1504
Copy link
Member

rom1504 commented Sep 20, 2025

I think we should rather disable a lot more than specifically physics during configuration phases

And it would be best to add a test for this (probably an internal test), so it will keep working

This is the main point

@tcortega
Copy link

I think we should rather disable a lot more than specifically physics during configuration phases

And it would be best to add a test for this (probably an internal test), so it will keep working

This is the main point

Yeah, I get that's the main point, but this PR shouldn't be blocked for it. This solves a real issue that's affecting me and other users right now.

If more things should be disabled, we can make a separate, more detailed issue for that. We shouldn't hold up this fix expecting the author to do it all here.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds handling for Minecraft's configuration phase by listening to start_configuration and finish_configuration events to control physics state during server configuration.

  • Disables physics when entering configuration phase
  • Re-enables physics when exiting configuration phase

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +454 to +455
bot._client.on('finish_configuration', () => { shouldUsePhysics = true })

Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Unconditionally setting shouldUsePhysics = true after finish_configuration may enable physics before the 'position' packet is received. The existing pattern (line 420) only enables physics after receiving the position packet to ensure the player position is properly initialized. Consider removing this line and relying on the 'position' packet handler to re-enable physics at the appropriate time.

Suggested change
bot._client.on('finish_configuration', () => { shouldUsePhysics = true })

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

Seems like good point @bmstefanski ? Position should be returned by server after entering play state

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.

5 participants