Bugfix/#2003 #1954 1.18 increased height

[greatmastermario]

Fixed issues where any chunk sections below y=0 and above y=255 would not render properly after being added in 1.18

Issues:
#1954
#2003

Screenshots of working render attached:

Red wool is from y 0-192, Green wool is from 193-255, and Blue wool is from 256-319

All sandstone is generated below y=0 (sections Y=0 to -4)

[greatmastermario]

Also fixes #2011

[ion1]

Maybe the spawnY < 0 check should also be updated to -64?

[greatmastermario]

Excellent point. I forgot to take that into account. Let me fix that.

[CounterPillow]

Thanks for your work on this!

There is however one problem: if I take a 1.17 map, render it before this change, then play on it and render it with this change, I get all blocks shifted down some positions:

I'm not sure if we can reasonably do anything about this.

[greatmastermario]

@CounterPillow Thank you for checking that. Is there a way for you to share the world download so I can download and test? I have a similar situation with one of my worlds, though in my case it is missing chunks. The 1.17 chunks that are rendering are properly aligned though.

[greatmastermario]

Also, just to cover our bases, did you recompile the C libraries? I would assume you did, but I want to be sure in case it's just an issue of a missed step.

[CounterPillow]

Yes, I did recompile the C libraries, Overviewer detects the changed version and wouldn't even have let me run it otherwise.

Here's the test world:
117signs.zip

It's important to render it first in overviewer git master, then launch the world, close it again so it writes some changes to the chunks as minecraft always does, then update the rendered map with your changes on top.

[greatmastermario]

Thank you. I didn't think of the fact that I changed the version.

So that I'm clear on the instructions:

1. Render the world (which is from 1.17) in overviewer master branch
2. Open the world in Minecraft 1.18 and then close
3. Render the world in overviewer bugfix branch

If those are the instructions, I have a feeling we will need to require --forceupdate or --check-tiles to ensure the tiles are updated. The logic is not backward compatible, so forcing an update should resolve this problem, and we will need to notify users of this fact.

[CounterPillow]

Step 2 is wrong, open the world in 1.17 so that it stays a 1.17 world. Other than that your test method is correct.

When I get the time I'll look into whether we can add a mechanism to forcerender if a map older than a certain version is detected.

[ion1]

I wonder if it would be feasible to map positions into tiles such that the blocks in 1.17 worlds and upgraded 1.18 worlds align correctly?

[greatmastermario]

I went ahead and added a version check to ensure there is a re-render on version update. It will still honor explicit render options, but it will default to forcerender if the version is changed or not present. I tested in local and it's working fine.

[CounterPillow]

Hmmm, I'm not a huge fan of this.

1. every overviewer update will prompt rerenders, which is a problem for huge worlds
2. findGitVersion might not work on source tarballs
3. this probably still means that the coordinates are wrong

I think what ion said is the proper fix here; rendered block 0,64,0 should remain at 0,64,0 even with the new height changes.

This may be a matter of just a simple offset somewhere in the chunks-to-tiles code that was forgotten.

[greatmastermario]

Got it. I will revert the last commit and continue to play with the offsets.

[BafDyce]

If wanted, I can also check the patch with my huge map! My world is close to 1GB in size (region folder), roughly 6k x 6k blocks and it contains chunks from various minecraft versions (oldest ones are from ~2011/2012). However, sharing world data (in case of incorrect rendering) will probably be impossible.

[greatmastermario]

@CounterPillow I am still working through the code, but the offset solution is proving to be... problematic at best. Unfortunately, it is not as simple as subtracting 4 from the row as I thought it would be. There are four interconnected methods, and the math simply isn't enough to take care of the issue.

I can do one of two things:

1. I can continue to work through this and see if I can resolve the missing chunks section issue
2. I can modify the --forcerender version logic to address your concerns

Please note I also was unable to replicate the same issue on my end. There was no offset issue when I followed the steps you gave (render, load in 1.17, update render).

[greatmastermario]

I went with option 2 for you to review. Adding the offset is proving to be very complicated. Here are the ways the new commit addresses the issue:

1. every overviewer update will prompt rerenders, which is a problem for huge worlds: I added an explicit TILESET_VERSION which will only be changed if there is a breaking change, similar to how overviewer.h does it
2. findGitVersion might not work on source tarballs: Adding the explicit version to the code will make sure the version isn't corrupted or lost
3. this probably still means that the coordinates are wrong: You are right, I have fixed that in the JS file and tested to make sure it's working now.

Please let me know if you have any other concerns.

[henu]

I tested this fix to my world. You can see the result here: http://storage1.henu.fi/Henulandia3

The converted chunks, which previously didn't work, now render fine below y=0. Old chunks are also fine.

The chunks between converted and old are not rendered for some reason, but I understood it is out of scope of this PR. They were not rendered with master branch either.

If you have other ideas how I could test this PR more, I'm happy to help if it's easy enough 😃

[greatmastermario]

@henu Thank you for the approval. I have found the issue with the missing chunks, but as you stated, I think that needs to be a separate PR/issue since it is a pre-existing issue in the master branch and not in scope for this PR.

[Wolfensteijn]

@greatmastermario Have you checked Nether rendering with your code?

The height of the Nether (and also the End) has not changed, so depending on what exactly you changed, there might be issues there. In fact, I am seeing top bedrock on my Nether rendering, which it didn't have before applying this patch.

Map can be found here: https://map.spiritiacraft.org/#/2/64/-3/-6/SpiritiaCraft_Nether%20-%20nether/SpiritiaCraft%20-%20Nether

[greatmastermario]

@Wolfensteijn I fixed it. Can you try again with the latest commit? You will need to rebuild the C libraries for the change to take effect.

[Wolfensteijn]

@greatmastermario yes, that fixed it!
https://map.spiritiacraft.org/#/-116/64/-138/-2/SpiritiaCraft_Nether%20-%20nether/SpiritiaCraft%20-%20Nether

[JakobDev]

The height limit can be increased with a data pack as AntVenom showed in his video, what means the height is not hardcoded in 1.18 as in the versions below. So I think it's not a good idea to use a hardcoded value in Overviewer. Players will maybe increase the height by data packs for larger buildings. And it should be possible to view the full building in Overviewer in that case.

[greatmastermario]

@JakobDev There are a couple problems with having a variable setup in Overviewer that need to be weighed to support variable height limits, and I think they need to be in a separate issue discussion so the admins for this repo can make a decision:

1. What is the scope for Overviewer worlds? Is it just vanilla and texture packs, or is it all possible mods and data packs? My inclination is that it is the former, and that since this is open-source we can modify the source for specific use cases outside the original scope. @CounterPillow Can you or someone else from the Overviewer org comment on this point?
2. Adding variable heights is technically feasible if we were just dealing with the overall height, but there are other things that need to be considered such as specific Y values relative to the tileset (such as spawn point) that would need to be determined by the data pack. Data pack support is currently not present in Overviewer, so this would need to be opened as an issue with a brand-new feature and development.

For now, my vote is for this fix to be included on its own, and we can talk about a variable world height feature in a separate issue and PR.

[dalandis]

I ran your edits for my map and see that the nether world is broken.

example http://map.gmgame.ru/#/-48/64/263/-5/gmgame_nether%20-%20nether/nether

Works if you turn on the roof. Probably due to height adjustment - Depth(min=0, max=50)

[greatmastermario]

@dalandis What do you mean by "height adjustment"? Is that a configuration option? Based on the prior discussion nether generation was tested and working.

If you can clarify the height adjustment you are referring to, I can test with that config.

Edit: Are you talking about the Depth option? If so, I will fix that. I think that is indeed broken looking at the code.

[dalandis]

yes, I'm talking about the depth options.

my config

nether_smooth_lighting = [Base(), Depth(min=0, max=50), EdgeLines(), Nether(), SmoothLighting(strength=0.5)]
renders["nether"] = {
     "world": "gmgame_nether",
     "title": "Nether",
     "rendermode": nether_smooth_lighting,
     "dimension": "nether",
     "crop": (-1562, -1562, 1562, 1562),
     'defaultzoom': 5
}

before the map looked like this

[greatmastermario]

@dalandis Fixed. You can pull the latest commit and it should work now. I tested on my world, and it works.

[dalandis]

I restarted rendering. But I can't confirm yet, we have to wait

[dalandis]

It seems to work. Thank you. It takes a very long time to complete rendering.

[KosmicAnomaly]

On the topic of hard-coding a max height, there are datapacks such as Amplified Nether which increase the height of the Nether, which then breaks all Nether renders. I think that the Nether roof should be able to be specified to solve this issue.

[wolfen351]

@CounterPillow Respectfully, could you please have a look at this and include it if it meets the quality standards?

[CounterPillow]

Merged as 03cb8ee, had to rebase to get rid of the one commit with its revert and couldn't push to your branch due to lack of access rights, so github will show this as "closed" now instead of "merged" but trust me, the commits are in master now.

Sorry it took this long to get around to this.