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

Render below zero retrogen chunks again #2051

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

Conversation

ralokt
Copy link

@ralokt ralokt commented Jun 24, 2022

Pre-1.18 chunks that are loaded in 1.18+ are often saved in a
transitional proto-chunk status.

Currently, the Overviewer will ignore these chunks altogether,
resulting in only part of the terrain being rendered. If the world is
optimized with --forceUpgrade, only chunks that have been visited in
1.18+ will be rendered.

With this change, the Overviewer detects these chunks and renders them
again.

Before:
Screenshot_20220623_113239

After:
Screenshot_20220624_190755

Before --forceUpgrade:
Screenshot_20220623_113213

Before update to 1.18:
Screenshot_20220623_113107

World for reference:
mcov-test-118chunks-118-forceupgrade.zip

I made the reference world by:

  • creating a new world in 1.17
  • teleporting to x/z=0/1024
  • teleporting back to 0/0
  • reopening the world in 1.18
  • and then optimizing it.

@ralokt ralokt changed the title Render chunks below zero retrogen chunks again Render below zero retrogen chunks again Jun 24, 2022
@cliffmeyers
Copy link
Contributor

cliffmeyers commented Jun 25, 2022

This addressed the issue I was seeing in #2038 (comment) where chunks not visited since upgrading to 1.18 would cause the render process to either 1. throw exceptions and eventually stall out, or 2. render the entire region as empty black tiles.

NOTE: the region being rendered here is r.2.-2.mca

Using latest master 8c4184d

chrome_day5bYbeQM

Using 1c6babc:

chrome_ydSg7oBJdY

Update: I have some more investigating to do - I rendered part of the 1.19 version of this world and ended up with a lot of blank spots. Hopefully tomorrow I'll have some time to test and create 1 or 2 sample worlds that repro the bug.

@ralokt
Copy link
Author

ralokt commented Jun 27, 2022

Repro would be much appreciated! I already have an idea what the issue might be - I might be too restrictive re: which chunks I select. Currently, only chunks with "heightmaps" or "spawn" as target status are being rendered. When I included "minecraft:heightmaps" in the list, all that produced was some additional completely dark chunks on the border, so I decided to leave it out - but if it turns out that they are required, it's trivial to include them. Interestingly, these dark chunks were exactly those that were rendered in the 1.17 version of the map, but weren't in the pre-upgrade 1.18 version (all the way on the left).

We do need to check the target status, as that allows us to exclude chunks that were proto-chunks before the upgrade. But perhaps an exclude list for statuses that are definitely border chunks makes more sense - even if some of them don't render properly, that is probably a lot better than them not rendering at all. Also, we should think about how to improve rendering in these cases anyway, if these cases aren't border chunks and not rendering isn't an option.

@ralokt
Copy link
Author

ralokt commented Jul 1, 2022

@cliffmeyers I'd love to take a look at the issues you had and try to fix them - if putting together a sample world is too much work, just an affected region file should be enough for me to keep working?

@cliffmeyers
Copy link
Contributor

cliffmeyers commented Jul 2, 2022

@ralokt sorry for the delay - hard to find time during the week sometimes. I appreciate your help.

Below is a subset of the world files (~50 MB). Every version upgrade has been run with --forceUpgrade and this world was upgraded to 1.19 most recently:

test-world-2.zip

MCA selector shows all regions as fully generated:

MCA_Selector_nDapZjs9Z9

Output of render from the world using 1c6babc:

chrome_Jm045rNbYh

The eastern edge correlates to the spawn in region 0/0. I haven't visited it since the 1.18 upgrade but am guessing there's some special handling to always render the spawn region? I have visited the western edge since the 1.18 upgrade and those are rendering normally.

Let me know if 1.19 might be throwing a curve ball and I can retrieve one of my 1.18 backups and extract the same subset.

@ralokt
Copy link
Author

ralokt commented Jul 4, 2022

Thanks! That will be invaluable for debugging :) I will probably only get to that in a few days, however:

  • I'm pretty sure the fully rendered spawn region is just due to the fact that the spawn chunks are always loaded, and therefore fully generated even if you never go there.
  • The lack of light everywhere is concerning, as that points to no light data in these chunks :/ Since I haven't seen that in my test world, I think this may well be a --forceUpgrade to 1.19 thing, as you predicted. Ofc this isn't confirmed, but a comparison to 1.18 would definitely be interesting, if you have the time!

@ralokt
Copy link
Author

ralokt commented Jul 9, 2022

A short update: Unfortunately, my laptop died, so I won't be able to work on this until end of next week.

@ralokt ralokt force-pushed the render-below-zero-retrogen-chunks branch from aacaff4 to 972a5be Compare July 24, 2022 08:23
@ralokt
Copy link
Author

ralokt commented Jul 24, 2022

Sorry for taking so long to get back to this - my new device took longer than expected to arrive...

Seems like we need those statuses prefixed with "minecraft:" after all - I've added them in a new commit. I've also rebased onto current master.

Now, all chunks in the testworld render - however, they are still completely dark.

Unfortunately, it seems like --forceUpgrade in 1.19 really does remove all skylight without regenerating it. I tried fullbrighting all chunks without any sections containing sky light data, and that took care of all chunks in the test world except for a ring of dark chunks around the spawn chunks. I plan on investigating these further.

But for now, I'd argue that fullbrighting these chunks would be scope creep. Rendering all chunks that are supposed to be rendered is already a huge improvement, even if they don't look pretty - the output is technically not wrong, either.

Also, I think reworking how dark chunks are detected and fullbrighted would be a good idea anyway.

So I'd prefer to fix the lighting in a separate PR - if preferred, I can also add a fix to that problem to this branch, though!

Pre-1.18 chunks that are loaded in 1.18+ are often saved in a
transitional proto-chunk status.

Currently, the Overviewer will ignore these chunks altogether,
resulting in only part of the terrain being rendered. If the world is
optimized with --forceUpgrade, only chunks that have been visited in
1.18+ will be rendered.

With this change, the Overviewer detects these chunks and renders them
again.
@cliffmeyers
Copy link
Contributor

Thanks for all your hard work @ralokt investigating this. Probably good to keep each PR tight and focused on a single issue as you suggested. Would the 1.18 version of that test world still be helpful or has identifying the skylight issue obviated the need for that?

@ralokt
Copy link
Author

ralokt commented Jul 28, 2022

You're welcome! I don't think the 1.18 version is necessary anymore :) In that case, I'll make another PR for the skylight change - it would probably make sense to merge that before this one.

Is there anything else you need from me to get this PR accepted?

@cliffmeyers
Copy link
Contributor

Nothing from me - but I'm not a committer and not familiar with this part of the code. Perhaps @CounterPillow could give a quick review and merge?

@ralokt
Copy link
Author

ralokt commented Aug 1, 2022

I don't think we should merge this PR in this state - we should merge #2060 first. These changes aren't compatible, but I already made a branch where I rebased these changes on #2060 . Just waiting for the other PR before I update this one :)

@ralokt ralokt marked this pull request as draft August 1, 2022 08:28
@ralokt
Copy link
Author

ralokt commented Aug 1, 2022

I turned this one into a draft for now!

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

Successfully merging this pull request may close these issues.

2 participants