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

fix block entity set #241

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

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jun 12, 2024

It never worked correctly.
not adding a test here as it's supposed to be used prismarine-provider-anvil obviously

@extremeheat
Copy link
Member

If it's an exposed API there should definitely be a test, even if it's used by anvil-provider

@zardoy
Copy link
Contributor Author

zardoy commented Jun 12, 2024

If it's an exposed API there should definitely be a test, even if it's used by anvil-provider

hm, but how do you suggest testing it without prismarine-provider-anvil? Should I just hardcode arguments in a function call in a test?

eg like

column.loadBlockEntities([
  {
    GlowingText: {
      type: "byte",
      value: 0,
    },
    keepPacked: {
      type: "byte",
      value: 0,
    },
    Color: {
      type: "string",
      value: "black",
    },
    Text4: {
      type: "string",
      value: "{\"text\":\"\"}",
    },
    x: {
      type: "int",
      value: 6,
    },
    Text3: {
      type: "string",
      value: "{\"text\":\"\"}",
    },
    y: {
      type: "int",
      value: -63,
    },
    Text2: {
      type: "string",
      value: "{\"text\":\"\"}",
    },
    z: {
      type: "int",
      value: 136,
    },
    Text1: {
      type: "string",
      value: "{\"text\":\"te\"}",
    },
    id: {
      type: "string",
      value: "minecraft:sign",
    },
  },
])

@@ -53,7 +53,6 @@ class CommonChunkColumn {
}

setBlockEntity (pos, tag) {
// Note: `pos` is relative to the chunk, not the world, tag's XYZ is
Copy link
Member

Choose a reason for hiding this comment

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

Why the removal of this comment? The blockEntities object is supposed to indeed hold entities relative to the current chunk. The chunk is not necessarily bounded to any world hence it's not supposed to hold global coordinates.

@rom1504
Copy link
Member

rom1504 commented Jun 16, 2024

this change seems incorrect since coordinates should be relative. Probably the issue is not here

@zardoy
Copy link
Contributor Author

zardoy commented Jun 16, 2024

this change seems incorrect since coordinates should be relative. Probably the issue is not here

yes, now I see, making coordinates be relative actually makes sense so they are linked to the chunk and not the actual world holding the chunk. I will make sure to fix it properly.

I also wonder if block entities also need to be dumped (using the dump method)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting for user input
Development

Successfully merging this pull request may close these issues.

3 participants