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

add the same block events as mineflayer #56

Merged
merged 2 commits into from
Feb 20, 2021
Merged

add the same block events as mineflayer #56

merged 2 commits into from
Feb 20, 2021

Conversation

rom1504
Copy link
Member

@rom1504 rom1504 commented Jan 1, 2021

see https://github.com/PrismarineJS/mineflayer/blob/master/docs/api.md#blockupdate-oldblock-newblock

working towards PrismarineJS/mineflayer#334 (comment)

this will do a get at each set, making the set methods a bit slower
it is needed to keep compatibility with mineflayer

I think it is ok as initialize can be used to do a faster set for many blocks

@rom1504 rom1504 requested a review from Karang January 1, 2021 23:58
@Karang
Copy link
Contributor

Karang commented Jan 2, 2021

prismarine-chunk is also doing a get internally for each set. Maybe we could have only 1 get per set by changing a bit the setBlock of prismarine-chunk to have it return the old block (it currently returns nothing).

https://github.com/PrismarineJS/prismarine-chunk/blob/master/src/pc/1.13/ChunkSection.js#L138

@rom1504
Copy link
Member Author

rom1504 commented Jan 2, 2021

yeah indeed that's an option
I see all the getBlockX are doing a getBlock before discarding most of the information. Initially these getBlockX were meant to be faster than getBlock by reading only what's needed.
Maybe we should just delete all the getBlockX and keep only getBlock and setBlock to simplify

@Karang
Copy link
Contributor

Karang commented Jan 2, 2021

yeah indeed that's an option
I see all the getBlockX are doing a getBlock before discarding most of the information. Initially these getBlockX were meant to be faster than getBlock by reading only what's needed.
Maybe we should just delete all the getBlockX and keep only getBlock and setBlock to simplify

wdym ? getBlockType and getBlockData are indeed alias for getBlockStateId, but the getBlock(Sky)Light are from different sources. imo the most important to have is getBlockStateId as getBlock is just a wrapper arround it (adding the instanciation of a Block object)

@rom1504
Copy link
Member Author

rom1504 commented Jan 2, 2021

ah yeah true light is from a different source
then the number of call to get block I added here cannot be decreased by moving the call to get block in pchunk because we don't do a get block call there

@Karang
Copy link
Contributor

Karang commented Jan 2, 2021

ah yeah true light is from a different source
then the number of call to get block I added here cannot be decreased by moving the call to get block in pchunk because we don't do a get block call there

What I meant is doing something like:
https://github.com/PrismarineJS/prismarine-chunk/blob/master/src/pc/1.13/ChunkColumn.js#L64

setBlock (pos, block) {
      let old = null
      if (typeof block.stateId !== 'undefined') {
        // assume each function is returning the old value
        let stateId = this.setBlockStateId(pos, block.stateId)
        old = Block.fromStateId(stateId, block.biome)
      } else {
        let stateId = this.getBlockStateId(pos)
        old = Block.fromStateId(stateId, block.biome)
      }
      if (typeof block.biome !== 'undefined') {
        old.biome = this.setBiome(pos, block.biome.id)
      } else { /*get*/ }
      if (typeof block.skyLight !== 'undefined') {
        old.skyLight = this.setSkyLight(pos, block.skyLight)
      } else { /*get*/ }
      if (typeof block.light !== 'undefined') {
        old.light = this.setBlockLight(pos, block.light)
      } else { /*get*/ }
      return old
    }

@rom1504
Copy link
Member Author

rom1504 commented Jan 2, 2021

yes I got it.
But in this PR, I added a getBlock in every setBlockX, where we don't do any getBlock currently, even in pchunk

@rom1504
Copy link
Member Author

rom1504 commented Jan 2, 2021

ah or do you mean replacing all the setBlockX by this setBlock ?

@rom1504
Copy link
Member Author

rom1504 commented Jan 2, 2021

I'll try to use this in mineflayer before merging

@rom1504 rom1504 added this to Needs triage in PR Triage Jan 15, 2021
see https://github.com/PrismarineJS/mineflayer/blob/master/docs/api.md#blockupdate-oldblock-newblock

working towards PrismarineJS/mineflayer#334 (comment)

this will do a get at each set, making the set methods a bit slower
it is needed to keep compatibility with mineflayer

I think it is ok as initialize can be used to do a faster set for many blocks
@rom1504 rom1504 merged commit 5f2702e into master Feb 20, 2021
PR Triage automation moved this from Needs triage to Closed Feb 20, 2021
@rom1504 rom1504 deleted the block_events branch February 20, 2021 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PR Triage
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants