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

Minecraft113 #1471

Merged
merged 47 commits into from Feb 20, 2019

Conversation

Projects
None yet
@gmcnew
Copy link
Contributor

gmcnew commented Aug 7, 2018

Fixes #1454!

For a future todo list, see this comment: #1454 (comment)

@agrif

This comment has been minimized.

Copy link
Member

agrif commented Aug 7, 2018

This is fantastic and quick work! I had another idea last night about how to avoid having to invent new block ids and make this all a bit easier to maintain, but I'd sooner get this merged and work on making it nice later.

The idea, FWIW: change the @material decorator to instead accept a block name and properties dict, then change it to automatically assign an internal block id to that combination. For old blocks before 1.13, have some extra info about how to translate from old block id + data into the new names, and from there into the new internal block id. The textures.py file would still construct an array mapping these internal ids to the block sprite to use.

At the end of it all, you could fairly quickly turn both the new paletted block data, and the old blockid + data system into an array of the new internal block ids, and then render using that. It would require only minimal changes to the C code, never requires maintainers to explicitly assign fake ids, and moves the name and properties of a block closer to its texture definition. It could also be changed only slightly to support mod blocks (if anybody still cares to do that).

Again: not something to worry about now. If nobody else takes a crack at it, I might. You've ton a huge bulk of the work here. I'll give it a test later this week and we can move forward.

@eminence eminence force-pushed the gmcnew:minecraft113 branch from b24eea5 to 01058ba Aug 7, 2018

eminence and others added some commits Aug 7, 2018

@mide

This comment has been minimized.

Copy link

mide commented Aug 7, 2018

Do we have tests? I know there has been a "Test World" Overviewer has used in the past - with blocks of many types and orientations. That may help us verify everything looks good.

Existing Tests: overviewer/Minecraft-Overviewer/test

Edit: It looks like TravisCI may be doing that and you're trying to fix that now.

@eminence eminence requested review from agrif and eminence Aug 7, 2018

@eminence

This comment has been minimized.

Copy link
Member

eminence commented Aug 7, 2018

We have exmaple, which is our standard test world, but it's not 1.13 compatible. If someone wants to undertake converting it, that would be awesome.

(Actually, we'd probably like 2 version -- a 1.13 compatible version and a 1.12 compatible version)

gmcnew and others added some commits Aug 7, 2018

Always interpret long_array as 64-bit
previously, some arrays could (by chance) be interpreted as smaller integers
ignore "decorated" chunks
This seems to fix lighting problems by ignoring chunks with no lighting (wiki suggests that "decorating" happens before "lighting")

RedSparr0w added some commits Sep 19, 2018

fix slab top/bottom slab
still not sure how to render double slab
@Plagiatus

This comment has been minimized.

Copy link

Plagiatus commented Oct 2, 2018

the win64 build works great on my windows PC. Is there a build for debian/ubuntu somewhere that I don't need to compile myself? or is there an ETA on an update to the package?
Love the work you guys are doing <3

@agrif

This comment has been minimized.

Copy link
Member

agrif commented Oct 9, 2018

@RedSparr0w Thanks for the PR, sorry it took so long to merge. It looks like it's mostly furnaces and dispensers left. Anybody know of any other blocks?

@Plagiatus no debian builds for this, sorry. If you really need them I can figure it out, but at the moment I'm scared a force-build will also install this branch in the debian repo.

@MrGraversen

This comment has been minimized.

Copy link

MrGraversen commented Oct 19, 2018

Really good job guys. Looking forward to this!

@Plagiatus

This comment has been minimized.

Copy link

Plagiatus commented Oct 29, 2018

@agrif i managed to build it myself in the meantime, don't worry.

a block i noticed missing is the quartz pillar.
and most stairs seem to always be in a default rotation and not in the one they actually have been placed in.

@matrixhacker

This comment has been minimized.

Copy link
Contributor

matrixhacker commented Jan 9, 2019

Updates on this anyone?

@mtoensing

This comment has been minimized.

Copy link

mtoensing commented Jan 10, 2019

Could somebody pls merge this? Or is this project finally dead?

@mtoensing

This comment has been minimized.

Copy link

mtoensing commented Jan 12, 2019

@CounterPillow
Wouldn't it be a good idea to merge this 113 branch into master? I saw some threads on reddit and boards around the internet where ppl ask why Overviewer does not work with Minecraft anymore. If they are lucky somebody points them to the 113 branch but in 2019 this is the most recent version. I would say you archive a "112" branch for all the ppl how wont update their servers but the majority would get the most recent version of Overviewer and Minecraft. What do you say? =)

@enaut

This comment has been minimized.

Copy link
Contributor

enaut commented Jan 12, 2019

@mtoensing
So far this is considered a alpha or beta... so you can use it at your own risk but there are still known problems. - just my 2$ ;)

@CounterPillow

This comment has been minimized.

Copy link
Member

CounterPillow commented Jan 12, 2019

I want to at least rewrite _packed_longarray_to_shorts in C or rewrite it in a less terrible way before merging this, since it is the main performance issue right now.

@mtoensing

This comment has been minimized.

Copy link

mtoensing commented Jan 12, 2019

@CounterPillow

rewrite packed_longarray_to_shorts

Can we help? Is this a task that takes 4 months or 4 days? Because I believe that the "missing" support for 1.13 damages the reputation of overviewer more than some bugs.

@strmtrupr2

This comment has been minimized.

Copy link

strmtrupr2 commented Jan 18, 2019

I was able to pull this branch and it worked with mscs and for my 1.13 world! Thank you :)

@agrif agrif merged commit 1428bf1 into overviewer:master Feb 20, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@Plagiatus

This comment has been minimized.

Copy link

Plagiatus commented Feb 20, 2019

Miracles do happen occasionally

@CounterPillow

This comment has been minimized.

Copy link
Member

CounterPillow commented Feb 20, 2019

Lies!

@agrif

This comment has been minimized.

Copy link
Member

agrif commented Feb 20, 2019

Ok, so I've merged this. Thank you everybody for your very hard work!

I believe there are a few extant blocks remaining. PRs for those will be merged soon. The remaining pain point is how slow the block unpacking code is. This could be improved by either rewriting it in C, or changing it to manipulate numpy arrays without iterating over them. I would prefer a numpy solution, but would not turn away either in a PR.

Again, thank you everybody for the work here. I never would have believed Overviewer would survive past 1.13, but here we are!

@mtoensing

This comment has been minimized.

Copy link

mtoensing commented Feb 21, 2019

Woho!

@piegamesde

This comment has been minimized.

Copy link

piegamesde commented Feb 21, 2019

The remaining pain point is how slow the block unpacking code is

May I point you to https://github.com/piegamesde/nbt/blob/develop/src/main/java/com/flowpowered/nbt/regionfile/Chunk.java#L231-L253? I don't know your performance requirements, but I haven't had any problems with this so far.

@agrif

This comment has been minimized.

Copy link
Member

agrif commented Feb 21, 2019

@piegamesde Thanks! but what we do now is essentially a version of that in Python, unrolled a little bit. The challenge is either rewriting it to use numpy arrays without any loops at all, or rewriting it in C and interfacing that C code to the python code.

And, of course, making sure the new code does the same thing as the old code.

@agrif

This comment has been minimized.

Copy link
Member

agrif commented Feb 22, 2019

I have rewritten the slow bit of code to be faster, but I want some verification it works for all of you before merging:

#1519

If anybody here could test it and report back (in the PR thread, please!) that would be very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.