Support all latest versions, add even more tests for other issues #79

zardoy · 2024-06-13T00:10:36Z

re opened pr as changed the branch

…ally)

@Nelonn

Contributed by @Nelonn

socket-security · 2024-06-13T00:10:47Z

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/prismarine-chunk@1.35.0)

View full report↗︎

extremeheat · 2024-06-13T00:12:50Z

Instead of closing and reopening an identical PR you should instead either do a force push where needed to change history or commits. This prevents the PR from needing to be re-reviewed from scratch.

zardoy · 2024-06-13T00:15:00Z

package.json

-    "prismarine-chunk": "^1.29.0",
-    "prismarine-nbt": "^2.0.0",
+    "prismarine-block": "^1.17.1",
+    "prismarine-chunk": "github:zardoy/prismarine-chunk",


blocked at least by PrismarineJS/prismarine-chunk#240

blocked by addressing comments at PrismarineJS/prismarine-chunk#240

zardoy · 2024-06-13T00:17:13Z

Instead of closing and reopening an identical PR you should instead either do a force push where needed to change history or commits. This prevents the PR from needing to be re-reviewed from scratch.

AFAIK, you cannot change the source branch after creating a Pull Request. You have to create a new one instead!

zardoy · 2024-06-13T00:18:43Z

all comments from the previous pr are resolved, now, I only need to think about what to do with failing tests: should I remove them or disable them somehow for now?

This reverts commit 5dbeb3b.

extremeheat · 2024-06-13T00:21:43Z

AFAIK, you cannot change the source branch after creating a Pull Request. You have to create a new one instead!

You cannot change your branch, but you can change the commits on the branch. If you need to change the commits on the branch and change history git makes it possible to update the history with a force push. If you need to make a new branch locally and push to a remote branch with your PR you do git push origin localBranch:remoteBranch --force.

all comments from the previous pr are resolved, now, I only need to think about what to do with failing tests: should I remove them or disable them somehow for now?

They are not resolved

zardoy · 2024-06-13T00:23:33Z

src/1.18/chunk.js

        const bitsPerBiome = Math.ceil(Math.log2(biomePalette.length))

        if (!bitsPerBlock) {
          blockStates = nbt.comp({ palette: nbt.list(nbt.comp(blockPalette)) })
        } else {
+          if (bitsPerBlock === 1 || bitsPerBlock === 2 || bitsPerBlock === 3) {


the test was already there, so I added it back:
https://github.com/PrismarineJS/prismarine-provider-anvil/actions/runs/9491519760/job/26157101354?pr=79

I don't understand. Are you saying tests in the current master branch are failing without this? I don't understand the reasoning behind this change, it doesn't make sense to me because this will definitely have an effect on decoding blocks (as in incorrect block states may be read despite tests seeming to pass).

But it is in the writing part of the code, not reading, isn't it? And yes tests are failing without this. Reverting this change and tests change will not fix the issue itself , for me saving didn't work most of the time. And it was the simple way to fix the problem . If you have any other suggestions please let me know 🙏

Also i couldn't find anything bad in this so far

This is definitely does not seem correct

zardoy · 2024-06-13T00:28:07Z

You cannot change your branch, but you can change the commits on the branch. If you need to change the commits on the branch and change history git makes it possible to update the history with a force push. If you need to make a new branch locally and push to a remote branch with your PR you do git push origin localBranch:remoteBranch --force.

Yes, the previous source branch is the default branch of my repo and I pushed some experimental unrelated to PR changes for my projects. In theory, I could rename it to something else, create a new branch, and set it as a new default branch with the old name. Sorry, I forgot there is a simple way to rename remote branches.

They are not resolved

Can you elaborate? I don't see anything that I made worse..

zardoy · 2024-06-13T00:32:04Z

tests are green, I will be waiting for your reply

extremeheat · 2024-06-13T04:48:12Z

test/savingLoading.js


 for (const version of testedVersions) {
+  if (todoVersions.includes(version)) continue


Why are you skipping versions? Either all versions should be tested or you should revert this change.

As I said in the previous PR all versions wasn't tested in the previous version of the code either. I just refactored it make it clear but didn't change the list of tested versions here

Remove the line

Or is your point that there is missing test data for these versions? If so that should be written explicitly in a comment

@extremeheat The point of this change is to ensure newly added versions in supported versions are tested without needing to add new versions here (you may forget to do it). This was made for convenience. Could I just rename it to skippedVersions? Or will it still not be clear?

extremeheat · 2024-06-13T04:50:42Z

test/chunkToNbt.js

+      if (!reChunk.dump().equals(chunk.dump())) {
+        console.warn('Warning: chunk dump does not match original chunk dump')
+      }


Why did you remove the assert here?

But what was the point of this assertion? Isn't it to okay to have a different buffer if it's valid? And this was failing for half of the versions.
Please don't say I disabled it just to make tests pass, I just really didn't understand the reason of testing it (especially when everything release works fine)

The point of the test is to ensure re-encoding works! There is no non-determinism in minecraft chunks.

ok, it will work, but again, what is the point of asserting the buffer contents? I don't think it's a good option to compare buffers without context.

eg it fails for previously disabled versions where re-encoding seems to be absolutely fine and the tests above also check it in a clear way.

I'm just trying to understand what is going on here. I want to enable more versions to test here, but this test fails and doesn't make any sense to me. Can you provide an example of the problem it solves?

extremeheat · 2024-06-13T04:51:13Z

test/chunkToNbt.js

+const todoVersions = ['1.9', '1.10', '1.11', '1.12', '1.15']
+const testedVersions = require('../').supportedVersions

 for (const version of testedVersions) {
+  if (todoVersions.includes(version)) continue


Same comment as below

extremeheat · 2024-06-13T04:52:29Z

src/1.18/chunk.js


    column.x = data.xPos
    column.z = data.zPos
    column.lastUpdate = data.LastUpdate.valueOf()
    column.inhabitedTime = data.InhabitedTime.valueOf()

    for (const section of data.sections) {
+      if (!section.block_states) continue


Is this valid? What's the reasoning behind this change?

This is also to fix saving test for the latest version. If the section doesn't have information what's the point to proceed it?

extremeheat · 2024-06-13T04:53:59Z

src/1.18/chunk.js

        const bitsPerBiome = Math.ceil(Math.log2(biomePalette.length))

        if (!bitsPerBlock) {
          blockStates = nbt.comp({ palette: nbt.list(nbt.comp(blockPalette)) })
        } else {
+          if (bitsPerBlock === 1 || bitsPerBlock === 2 || bitsPerBlock === 3) {


I don't understand. Are you saying tests in the current master branch are failing without this? I don't understand the reasoning behind this change, it doesn't make sense to me because this will definitely have an effect on decoding blocks (as in incorrect block states may be read despite tests seeming to pass).

extremeheat · 2024-06-13T04:54:38Z

Yes, the previous source branch is the default branch of my repo and I pushed some experimental unrelated to PR changes for my projects. In theory, I could rename it to something else, create a new branch, and set it as a new default branch with the old name. Sorry, I forgot there is a simple way to rename remote branches.

No, "renaming" the branch will either auto close the PR or move the PR's source branch with it. The only way to fix it is to keep the current remote and push a new history to it. I don't see anything different in this PR to the last one, but I've re-reviewed it.

zardoy · 2024-06-13T12:14:27Z

I don't see anything different in this PR to the last one, but I've re-reviewed it.

I already pointed that I couldn't keep the same remote, it was not because of different history. At least I made tests green in this PR. And yes, not all was tested on master (and I increased the number of tested versions in writeNBT), that's why there are some of the changes to fix them

rom1504 · 2024-06-16T21:25:06Z

until #75 if fixed, it won't be possible to support newer versions

zardoy · 2024-06-16T21:26:33Z

until #75 if fixed, it won't be possible to support newer versions

how is this related? new versions almost don't have any changes and will work with 1.18 implementation fine (just like in p-chunk)

…files

rom1504 · 2024-09-08T20:06:15Z

please address comments or close the PR

zardoy · 2024-09-08T21:32:12Z

please address comments or close the PR

easy to say...

zardoy and others added 19 commits August 30, 2023 06:13

require lazily

0308af9

make created worlds loadable in singleplayer

8d53ef2

add typings field

7f35ee4

Merge branch 'level-dat' into everything

be236eb

1.8 blockEntities support

ce3de84

Merge branch 'block-entity' into everything

4a84b08

fix reading format

9b3d4b9

Merge branch 'block-entity' into everything

49bf815

general block entity support for all versions (didn't check 1.18 actu…

b250d93

…ally)

levelname & allowCommands support

44bb47f

fix level typing

6676ce9

another fix for levels

0ddcd9d

1.18 saves fixes

02d81b0

Merge remote-tracking branch 'upstream/master' into everything

a61f00f

should not be filtered

6c7e390

Support all latest versions (#1)

422aed5

Contributed by @Nelonn

fix test, now make it worse

dd67a07

even more failing tests!

2393e9e

remove unused (anymore) block entities handling

3fda260

undo changes in anvil

007cb8e

zardoy commented Jun 13, 2024

View reviewed changes

undo more changes

5dbeb3b

Revert "undo more changes"

4300a8d

This reverts commit 5dbeb3b.

zardoy commented Jun 13, 2024

View reviewed changes

remove file for another pr

c1d642c

extremeheat requested changes Jun 13, 2024

View reviewed changes

important! fix compatibility with some versions of new region format …

2625c27

…files

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support all latest versions, add even more tests for other issues #79

Support all latest versions, add even more tests for other issues #79

zardoy commented Jun 13, 2024

socket-security bot commented Jun 13, 2024 •

edited

Loading

extremeheat commented Jun 13, 2024

zardoy Jun 13, 2024

rom1504 Sep 8, 2024

zardoy commented Jun 13, 2024

zardoy commented Jun 13, 2024

extremeheat commented Jun 13, 2024 •

edited

Loading

zardoy Jun 13, 2024

extremeheat Jun 13, 2024 •

edited

Loading

zardoy Jun 14, 2024 •

edited

Loading

extremeheat Jun 16, 2024

zardoy commented Jun 13, 2024

zardoy commented Jun 13, 2024

extremeheat Jun 13, 2024

zardoy Jun 13, 2024

extremeheat Jun 16, 2024

extremeheat Jun 16, 2024

zardoy Jun 16, 2024

zardoy Jun 16, 2024

extremeheat Jun 13, 2024

zardoy Jun 13, 2024

extremeheat Jun 16, 2024

zardoy Jun 16, 2024

extremeheat Jun 13, 2024

extremeheat Jun 13, 2024

zardoy Jun 13, 2024

extremeheat Jun 13, 2024 •

edited

Loading

extremeheat commented Jun 13, 2024

zardoy commented Jun 13, 2024 •

edited

Loading

rom1504 commented Jun 16, 2024

zardoy commented Jun 16, 2024

rom1504 commented Sep 8, 2024

zardoy commented Sep 8, 2024


		for (const version of testedVersions) {
		if (todoVersions.includes(version)) continue

Support all latest versions, add even more tests for other issues #79

Are you sure you want to change the base?

Support all latest versions, add even more tests for other issues #79

Conversation

zardoy commented Jun 13, 2024

socket-security bot commented Jun 13, 2024 • edited Loading

extremeheat commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zardoy commented Jun 13, 2024

zardoy commented Jun 13, 2024

extremeheat commented Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

extremeheat Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

zardoy Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zardoy commented Jun 13, 2024

zardoy commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

extremeheat Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

extremeheat commented Jun 13, 2024

zardoy commented Jun 13, 2024 • edited Loading

rom1504 commented Jun 16, 2024

zardoy commented Jun 16, 2024

rom1504 commented Sep 8, 2024

zardoy commented Sep 8, 2024

socket-security bot commented Jun 13, 2024 •

edited

Loading

extremeheat commented Jun 13, 2024 •

edited

Loading

extremeheat Jun 13, 2024 •

edited

Loading

zardoy Jun 14, 2024 •

edited

Loading

extremeheat Jun 13, 2024 •

edited

Loading

zardoy commented Jun 13, 2024 •

edited

Loading