search

AllenSeitz / DimDungeons

A Minecraft mod which adds proceedurally generated dungeons.
14 stars 11 forks source link

Add compatibility for the mod Lootr, simplify loot table setting. #16

Closed noobanidus closed 3 years ago

noobanidus commented 3 years ago

Using the setLootTable member method of a tile entity prevents the mod Lootr, which replaces loot chests with a chest that creates an inventory for each player that opens it (making it possible for multiple people to loot the same chest and all get loot), from being able to hook in.

While a hook can be made into the tile entity, generally the point at which the setLootTable call is made is using the IWorld instance of chunk generation, which isn't the same as the world found in the tile entity (if it even has one).

Conversely, using LockableLootTileEntity.setLootTable provides the chunk generation world and serves as a convenient point for injection. It otherwise replicates the functionality as you had it: fetching the tile entity, calling the function and setting a random seed.

Where you also had error testing, I added an additional test that would trigger your debugging message instead.

Lootr currently supports all vanilla loot chests, including the ones that are really annoying, and uses a structure processor to convert all structure NBT files as they're loaded from disk.

Previous attempts to use a tile tracker and determine if a chunk was ready for external modification (i.e., generation was complete) proved ineffectual and while some aspects worked well, there were too many concurrent modification errors in 1.15.

Finally, I'm not sure if I've got the formatting right, but there doesn't seem to be a huge consistency to the formatting so I tried to just match what was around.

My only constructive criticism would be that there's a lot of repetitive functions -- you have two "fillChestBelow" and "fillBarrelBelow" functions with almost identical code that could just be static methods.

AllenSeitz commented 3 years ago

I see the problem you're having. In 1.16.4 I changed it so that portals are no longer built during chunkgen, but instead at the moment that a key is placed in the keyhole. I wonder how that potentially affects you?

But this patch looks like it is intended for 1.15.2 right? Looks good. Honestly I've given up on all but the 1.16 branch at the moment but I will push easy changes to this older branch when I can. There are three completely separate code bases for this mod: 1.12 (broken, unreleased), 1.14-1.15, and 1.16. And this mod was a complete rewrite between each of them. So I think what I'll do is make a new 1.15.2 branch off master, merge this into it, and upload it to curseforge as a beta for 1.15.2.

You're right about the style comments. My formatting has improved in the 1.16 branch (or I hit ctrl-shift-f more). And I like using LockableLootTileEntity instead. The reason I needed separate data blocks for chests, barrels, and trapped chests, is because I was getting the tile entity and calling set loot table on that. And of course if I try to get the BarrelTileEntity at the position of a chest it will return null. But I think I'll change this in 1.16 and make all those data blocks synonymous. Thanks for the tip.

noobanidus commented 3 years ago

Oh. Bugger. For some reason it didn't even occur to me that the default branch wasn't the 1.16 branch. I'll close this pull (I totally understand not wanting to support 1.15.2) and check out your 1.16 code instead.

AllenSeitz commented 3 years ago

Oh you weren't trying to support 1.15.2? I think I'll still merge this anyway, to both 1.15.2 and 1.16.4, because you have a point there with LockableLootTileEntity.

noobanidus commented 3 years ago

Yeah, it was meant to be for 1.16.2, specifically to add compat for Lootr.

AllenSeitz commented 3 years ago

Wait 1.16.2? Is that a typo of 1.15.2? You still need it for 1.16.4 right?

noobanidus commented 3 years ago

Typo, my bad. This is what I get for trying to reply to issues first thing in the morning. And creating them late at night.

noobanidus commented 3 years ago

If it would help I'm willing to make a patch for your 1.16 branch as well.

AllenSeitz commented 3 years ago

Nah it's cool. I copy/pasted the same 10 lines of code into 1.16 and it seems like they worked just fine. I tested it for 5 minutes with Lootr and it seems like it just worked. That easy. I gave you credit on the CurseForge page. (And I uploaded 1.091 with this in it.)

AllenSeitz commented 3 years ago

Minor glitch actually - Lootr only converts half of a double chest. But that's a Lootr problem, and I don't think there are any naturally spawning double chests in vanilla so that's fair.

noobanidus commented 3 years ago

Yeah, unfortunately that is an issue, I haven't programmed in any checks for double chests. If that's something that you actually do, however, it's something I can code in support for -- it would just be a matter (potentially) of detecting a double chest and converting the relevant adjacent block also.

(It would be a matter of my replacement code checking for a ChestTileEntity as well and then checking for whether or not it's a single or a double. Like you said, all vanilla are singles, so that's the default; I don't think I would go to the extent of making a double chest model though, it would just be two chests side-by-side.)

Also, no need for credit! This is my mod after all that I'm asking people to support -- I'm just trying to do the leg work so that supporting it isn't a pain in the butt.