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

feat: Add optimistic update for block placement #3391

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jun 6, 2024

This is what native client does. It is important if the ping is low and after placement, calculates physics correctly. If the placement is not allowed the block is still updated to air by the server.

Maybe it's worth adding it as a setting to not break existing behaviors, but I don't see a reason why it shouldn't be enabled by default .

The code changes in `place_block.js` introduce an optimistic update feature for block placement. This feature sets the default state of the block being placed based on the held item. It retrieves the default state from the `minecraft-data` module and updates the block state at the destination accordingly. This optimistic update improves the responsiveness of block placement.

Note: This suggested commit message follows the convention of using a prefix to indicate the type of change (`feat` for a new feature).
Copy link
Member

@extremeheat extremeheat left a comment

Choose a reason for hiding this comment

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

This is what native client does

What version does this apply to and do you have a source for that?

Comment on lines +12 to +17
const heldItem = bot.heldItem
const mcData = require('minecraft-data')(bot.version)
const defaultState = mcData.blocksByName[heldItem?.name]?.defaultState
if (defaultState) {
bot.world.setBlockStateId(dest, defaultState)
}
Copy link
Member

Choose a reason for hiding this comment

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

  1. This should be using the registry
  2. This is not a correct way to deduce correct block state from an item
  3. If world state is mutated then you must ensure that the data is fully in sync with the server (how does this handle the case where the server rejects?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I already answered 3rd question in the description. If the server rejects a block update would be emitted (even if it's air) as this is what the native client also does.

Can you elaborate on the 2nd question?

@zardoy
Copy link
Contributor Author

zardoy commented Jun 7, 2024

What version does this apply to and do you have a source for that?

I don't know how to find things in the native client source I could check it by launching the client. This was always the behaviour of minecraft client.

@rom1504 rom1504 added this to Needs triage in PR Triage Jun 16, 2024
@rom1504 rom1504 moved this from Needs triage to Waiting for user input in PR Triage Jun 16, 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
  
Waiting for user input
Development

Successfully merging this pull request may close these issues.

None yet

2 participants