Thaumic-Tinkerer / ThaumicTinkerer

A Spiritual Successor to the Elemental Tinkerer mod. This time Thaumcraft flavoured.
142 stars 121 forks source link

actually fix cleansing talisman breaking #1076

Closed Midnight145 closed 11 months ago

Midnight145 commented 11 months ago

This fixes the bug referenced here with the talisman never actually breaking

Sorry for the double commit on the issue, I realized my branch was screwed up and had to recreate it

GaeaKat commented 11 months ago

please do not try random code. if you look at the part you subtracted one from, thats the TYPE of the bauble, not the number. you are basically saying there, delete whatever object is below an amulet in the enum.

Midnight145 commented 11 months ago

This is tested and working on my end.

GaeaKat commented 11 months ago

Cool, looking into it it looks like we were both wrong. getBaubles() returns an IInventory - https://github.com/Azanor/Baubles/blob/1.7.10/src/main/java/baubles/api/BaublesApi.java#L19 as seen here so it should have been simply 0 not the ordinal OR ordinal - 0

GaeaKat commented 11 months ago

My apologies for the sharpness, It is 6 am, I should be sleeping but the day has been a hard one

Midnight145 commented 11 months ago

No worries, I get it haha. I'll update that in the code for clarity real quick.

GaeaKat commented 11 months ago

Wait, no https://github.com/Azanor/Baubles/blob/1.7.10/src/main/java/baubles/common/container/InventoryBaubles.java the stack number IS the one needed, this needs looking into more

GaeaKat commented 11 months ago

It looks like yeah, you are subtracting 1 from the slot TYPE, not the number, so why it is working is confusing, Lemme quickly check if java enum's start from 0 or 1

Midnight145 commented 11 months ago

I think that it's just indexed 0 to 3, top to bottom in-game. Just going by bauble type wouldn't work because there are two ring slots.

Midnight145 commented 11 months ago

I am definitely subtracting from slot type on accident, I didn't take a close enough look at BaubleType to realize that it didn't correlate directly to the inventory slots

GaeaKat commented 11 months ago

yeah, InventoryBauble does have 4 slots, and there are 3 types, I am guessing there is some magic numbering going on here I shouldnt be doing

GaeaKat commented 11 months ago

Double checking, Amulet is ALWAYS 0, so just 0'ing that out should be best!

GaeaKat commented 11 months ago

https://github.com/Azanor/Baubles/blob/1.7.10/src/main/java/baubles/common/container/InventoryBaubles.java#L200

Midnight145 commented 11 months ago

https://github.com/Azanor/Baubles/blob/master/src/main/java/baubles/api/BaubleType.java#L3

Right here has indices for each bauble type

GaeaKat commented 11 months ago

https://github.com/Azanor/Baubles/blob/master/src/main/java/baubles/api/BaubleType.java#L3

Right here has indices for each bauble type

Need to make sure you are on the right branch, TT would only work with the 1.7.10 stuff after all

GaeaKat commented 11 months ago

https://github.com/Azanor/Baubles/blob/1.7.10/src/main/java/baubles/api/BaubleType.java sadly much less helpful in 1.7.10

Midnight145 commented 11 months ago

oof, I completely forgot to switch branches. That would explain why I didn't see it in the api.

GaeaKat commented 11 months ago

Looks like as said above, for amulet it is always 0 there

GaeaKat commented 11 months ago

But yeah I hope you can see how with the OLD bauble API, this was a lot more confusing, Though I have no idea how this snuck past when I had tested it as working.

Midnight145 commented 11 months ago

Oh, yeah, the documentation is not great. I missed it either, someone had reported it as a Blightfall issue previously.

GaeaKat commented 11 months ago

Once the checks have finished will accept the restyled, I THINK that keeps your commit in there

Midnight145 commented 11 months ago

Sounds good!

GaeaKat commented 11 months ago

Sounds good!

Committed into the base, sorry again for initial message, 6 am and honestly when I looked at it, it was, Why would subtracting 1 from an enum ordinal fix this bug???111?? it has obviously been mistaken for the stack size. Will teach me to assume , so my Apologies

GaeaKat commented 11 months ago

Hopefully 1.7.10 will be easier to make a new build for then 1.6.4 was

Midnight145 commented 11 months ago

You're totally fine, no worries. I probably wrote that code initially around 6am, I probably mistook it for a 1-based index or something without looking into it too much.

Midnight145 commented 11 months ago

Yeah, I'm still not really sure what black magic I pulled off to make my dev environment work, but I was actually able to build. Don't even want to think about building for 1.6.4 though

GaeaKat commented 11 months ago

Yeah, I'm still not really sure what black magic I pulled off to make my dev environment work, but I was actually able to build. Don't even want to think about building for 1.6.4 though

For 1.6.4 I had to in the end, decompile, make my change, and byte-code edit JUST the changed file, with an old copy of javac to compile the extra file on an old laptop, and slip it into the jar then hex-edit a different file that the byte-code editor wouldnt touch . Nothing I did got a dev-env working.

Midnight145 commented 11 months ago

I've thrown some files in recaf before to test some quick hacky fixes without having to fully set up a dev environment for a mod, but in general I haven't had too many issues

GaeaKat commented 11 months ago

It was to fix the SerializationIsBad issue, so I had to replace use of ObjectInputStream with a version that filtered out everything but the classes I used.

Midnight145 commented 11 months ago

Ohh, yeah. I'm glad that I've never had to use ObjectInputStream, I really don't want to have to deal with the headache.

GaeaKat commented 11 months ago

Those many years ago, it was in every "Here's how to do networking in Forge" article as it was so easy to use etc

Midnight145 commented 11 months ago

Huh, I should probably make sure that it's never used in any of the mods I "inherited" from maintaining blightfall then. I know I've never used it personally, and I don't think I've seen it anywhere, but I should probably double-check.

GaeaKat commented 11 months ago

Main places to be worried is in networking code ofc. If it is used to save files to and from disk that is less worrysome. One of the reasons I prefer SerialisationIsBad as compared to the other fix for it.

Midnight145 commented 11 months ago

Did a recursive grep for the one mod that it would be an issue, thankfully I didn't find anything.

GaeaKat commented 11 months ago

1.7.10 added in ByteBuf and a lot of people moved to that instead as it was a lot easier to use imo

Midnight145 commented 11 months ago

Yeah, thankfully all of the code I work with just uses ByteBuf instead