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

Basic port to 1.19.2 #467

Closed
wants to merge 22 commits into from
Closed

Conversation

sisby-folk
Copy link
Contributor

@sisby-folk sisby-folk commented May 15, 2023

Hello again! Been a bit.

1.19.2 is mostly friendly, with a bit of syntax changes.

Two major bumps along the way

  • StructureFeature is now StructureType and is slightly different, leading us to use StructureType.JIGSAW for the village marker, which is okay but bastions are gonna show up as funny little houses. Other option would be to flag each village type manually, but that doesn't work for modded villages. Maybe something APIs have solved.
  • Biome categories just.. aren't a thing anymore! You don't get biome information on init at all, which makes sense considering datapack biomes never had this (if i remember right). Anyway, this basically leads to us killing guessFittingTextureSetFallback (so it'll just use tags), and using tags instead where possible (like for priority for rivers, or checking whether something is nether). This means I've cached less, so if this is intensive that's worth a look.

Besides that, pretty smooth, had to do a few toolchain bumps but it worked out.

javaw_qX1BExzM4n
javaw_F3ESKwi0SS

@sisby-folk
Copy link
Contributor Author

For your own interest, we also made an itemless fork that we're planning to use for our no-modded-items modpack

@tyra314
Copy link
Member

tyra314 commented May 15, 2023

Thank you for the PR, I'll check it out.

For your own interest, we also made an itemless fork that we're planning to use for our no-modded-items modpack

No need to fork for that. Change itemNeeded config entry to false. If that doesn't work, patches are welcome.

@sisby-folk
Copy link
Contributor Author

In this specific case (Tinkerer's Quilt) the entire modpack hinges on not adding items to the registry / world at all - it was fun to comb through all of that code though!

@sisby-folk
Copy link
Contributor Author

sisby-folk commented May 16, 2023 via email

@sisby-folk
Copy link
Contributor Author

sisby-folk commented May 17, 2023

False alarm! I actually had yttr installed, which replaces nether biomes in some way - they show fine with vanilla!

@sisby-folk
Copy link
Contributor Author

sisby-folk commented May 17, 2023

More notes as we've gone along.

  • Texture sets that need adding in 1.19 include:
    • mangrove_swamp
    • lush_caves
    • dripstone_caves
    • deep_dark
  • We have no idea how to disable debug mode in builds (oops) (oh, it's config)
  • We wrote a fwaystones addon, which required manually mapping waystone hash strings to marker id ints (and saving the map persistently) - we might eventually ask about letting marker IDs be arbitrary or something.
  • While debugging, we've occasionally experienced S2C tile packets fail, causing filled atlases to get stuck blank. Seems timing and architectury related, but we can't reproduce it consistently. Yet to see it in a production environment like our pack.
java.lang.NullPointerException: Cannot invoke "net.minecraft.class_1657.method_5770()" because the return value of "dev.architectury.networking.NetworkManager$PacketContext.getPlayer()" is null

   at hunternif.mc.impl.atlas.network.packet.s2c.play.MapDataS2CPacket.lambda$apply$0(MapDataS2CPacket.java:42) ~[transformed-mod-antiqueatlas.i0:0/:?]

@tyra314
Copy link
Member

tyra314 commented May 23, 2023

* While debugging, we've occasionally experienced S2C tile packets fail, causing filled atlases to get stuck blank. Seems timing and architectury related, but we can't reproduce it consistently. Yet to see it in a production environment like our pack.

Yeah, I've seen this shit as well. Really annoying. Didn´t notice though that it only occurred in dev environments. I could have sworn that I had a bug report for that.

Copy link
Member

@tyra314 tyra314 left a comment

Choose a reason for hiding this comment

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

Thanks for the work. There's quite a bit left, though.

Once it's ready, I'll create a new branch '1.19' as target.

@sisby-folk
Copy link
Contributor Author

@tyra314 on the "stuff failing to send to the client thing" - I think you might be right and it might be happening on prod too? I thought it was just a mod conflict, but I'm occasionally getting netty exceptions on both the client and server, and I've been experiencing a bug (which I thought was unrelated) where markers in the nether would get completely wiped. Not too sure there!

Here's the log bug that makes maps totally blank, though:

image

I think it happens consistently if you log out / save while holding an atlas in your hand. That would imply the game is checking for atlases before the player is actually initialized properly.

@sisby-folk sisby-folk requested a review from tyra314 May 24, 2023 01:06
@tyra314
Copy link
Member

tyra314 commented May 24, 2023

I think it happens consistently if you log out / save while holding an atlas in your hand. That would imply the game is checking for atlases before the player is actually initialized properly.

Well, at least that is a good starting point to reproduce the issue.

@Daenyth
Copy link

Daenyth commented Jul 13, 2023

Is there any way I can help with this?

@sisby-folk
Copy link
Contributor Author

sisby-folk commented Sep 11, 2023

@tyra314 hi hello! I'm at the point where I'm thinking about poking 1.20.1 with a stick, any further thoughts on this one? (besides not being able to display underground biomes, naturally)

@Daniilnm
Copy link

hello, I want to ask how the work is progressing, there have been no changes in this branch for a long time

@Daniilnm
Copy link

@tyra314 hi hello! I'm at the point where I'm thinking about poking 1.20.1 with a stick, any further thoughts on this one? (besides not being able to display underground biomes, naturally)

.

@tadhgmister
Copy link

@sisby-folk, I tried using the artifact from this pull request but ran into an error:

java.lang.IllegalAccessError: 
class hunternif.mc.impl.atlas.client.TileTextureMap tried to access private method
'boolean hunternif.mc.impl.atlas.client.forge.TileTextureMapImpl.biomeIsJungle(net.minecraft.core.Holder)'

And looking at the file, biomeIsJungle is the only private method of the lot, every other one is public and there was a commit 3 months ago where you fixed visibility for several other methods but jungle was left out.

It crashes as soon as I try to load a world using forge, I'm not sure how to suggest an edit, do you think you could fix that in the correct place to change it?
Thanks :)

Copy link

@tadhgmister tadhgmister left a comment

Choose a reason for hiding this comment

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

crashes for me immidiately upon joining a world in forge because it needs this to be public, I assume this was an oversight.

@sisby-folk sisby-folk mentioned this pull request Oct 14, 2023
@TheOverpassArsonist
Copy link

What's the status of this? As far as I can tell issue that's been mentioned has already had commits addressing it/them, and every item in the requested changes by tyra show as resolved. Is this done and it's just a matter of publishing it, or are there remaining issues? (for reference, not a mod developer myself, but I checked in to see what the progress was at and I noticed that it seems like everything that's been mentioned has been addressed)

The one thing that github is citing as an issue is the requested changes, but as far as I can tell all of the requested changes have been made. On sept 11th this comment was made implying it was solid about a month and a half ago and (aside from one private/public mixup) it doesn't look like any other issues have cropped up.

Not trying to poke or prod for some ETA/date just curious as to the general state this PR is in since it appears as though every fix that was requested/needed has been made already.

@sisby-folk
Copy link
Contributor Author

sisby-folk commented Dec 22, 2023

Hey hello! @tyra314 just a heads up that we're planning to put our itemless forks of this PR (and the 1.20.1 one) on modrinth within a week or so. We weren't originally planning on it, but someone else went and did it without asking or notifying us, and we'd like to avoid that happening a second time.

We'll be forgoing any monetization on modrinth and crediting clearly to suit the cc-nc-by-sa textures.

Naturally, let us know if there are any issues, and as always you're still perfectly welcome to adapt any of our changes in these PRs into the main mod and MR project. Cheers!

@sisby-folk sisby-folk closed this Jan 6, 2024
@sisby-folk sisby-folk deleted the 1.19 branch January 6, 2024 03:20
This was referenced Jan 6, 2024
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.

6 participants