MachineMuse / MachineMusePowersuits

Minecraft mod, take 2
237 stars 105 forks source link

[1.7.10] Galacticraft conflict #905

Open krallendertoten opened 5 years ago

krallendertoten commented 5 years ago

When in a Galacticraft space station, power armor will randomly gain tens, and sometimes hundreds of thousands of heat units, even when just standing still.

picture of issue: http://imgur.com/a/YgPuGaF

This issue is occurring in the Voltz Community Modpack

lehjr commented 5 years ago

Unfortunately, i have no idea what version of Numina they are using, other than it was something that was "patched" rather than filing a bug report.

lehjr commented 5 years ago

The version of Numina they are using has Scala code in it, and Numina hasn't had Scala code in it since 2017. This is supposed to be their patched version code here: https://github.com/kmecpp/Numina/tree/1.7.10-Java But there's no scala code in that, so I have no idea what they did and cannot support this build or combination.

lehjr commented 5 years ago

If you can reproduce the issue with official builds, then I will look into it.

kmecpp commented 5 years ago

Hi, I am the author of that Numina build and there is no Scala code in that version.

I literally just took the 1.7.10 java version https://github.com/MachineMuse/Numina/tree/1.7.10-Java and added this one commit: https://github.com/kmecpp/Numina/commit/e86833a1b85ac68c9fd835909b648bf89cf7de63

I submitted a PR but it was never merged or commented on https://github.com/MachineMuse/Numina/pull/24

lehjr commented 5 years ago

Yeah, there actually scala in the build that is downloaded by that modpack. I'm sitting here looking at it.

lehjr commented 5 years ago

Numina-0.4.1.0-kmecpp.jar There are several scala classes and traits.

kmecpp commented 5 years ago

Huh. I had no idea lol. I guess I compiled the patch with the wrong branch or something. I will change it to the Java version now.

lehjr commented 5 years ago

basemod.NuminaProxy (trait) basemod.NuminaProxyClient (scala class) basemod.NuminaProxyServer (scala class)

item.JsonItemBase (scala object) item.NuminaItemBaser (scala trait)

network.ReadMonad (trait and object)

and several more

lehjr commented 5 years ago

The PR was likely never noticed due to Numina barely ever having any activity, that and the intent at not continuing to support 1.7.10 due to its age and due to there being no modder support from Forge for anything remotely that old. True story, go to Forge forums and ask for anything on 1.7.10, thread locked.

For 1.12.2 and beyond, I'll be using my own fork since I don't have full control over the repo and can't set default branches.

kmecpp commented 5 years ago

That is understandable. I think I found the cause of the scala issue. I have the 1.7.10-Java branch cloned and all the source files are pure java, but when I run gradlew build it outputs a jar with the scala classes. Have any idea why that would be happening?

lehjr commented 5 years ago

Tried the main repo just to make sure there was nothing weird going on there. Just tried your fork and no scala there either. I would try a clean checkout and go from there.

kmecpp commented 5 years ago

Weird. That didn't work. Had to delete the whole workspace and rebuild everything. I will update the modpack now and ask the OP to update this issue if its fixed or not.

lehjr commented 5 years ago

Likely not fixed, but it's better to be able to rule out anything odd.

lehjr commented 5 years ago

I'll have to debug this before I can figure it out. Can't remember the last time I set up a GC space station.

lehjr commented 5 years ago

Wasn't able to reproduce. However, there are only a few points where heat is applied.

That's it. So my guess is that there's some type of heat based damage getting picked up by the armor. And/Or it could be related to this: https://github.com/MachineMuse/MachineMusePowersuits/issues/731

lehjr commented 5 years ago

I finally got a chance to try that code change in Numina. @kmecpp you do know the ENTIRE POINT of the FOV handler is to turn off the FOV change when sprinting, right? Simply disabling it in the config is enough. All you've managed to do is disable what the code is supposed to do in the first place.

kmecpp commented 5 years ago

I forget how the code works looking at it 6 months later but it functions fine.

Prior to my patch, The sprint FOV change is completely disabled as you say. However if you disable this "fix" in the config, then the FOV changes to crazy high values when sprinting with PA on. This is why I assume the FOV was added in the first place.

What the patch does is make it so that when you sprint, it changes the FOV value to the normal setting instead of disabling it entirely. That way there aren't crazy FOV values as there would be without the FOV handler but the sprint animation still works.

lehjr commented 5 years ago

I know what the patch does. First, the sole purpose of the FOV change the code is meant to address is when the player is sprinting, so again, your code disables that.

Second, there are people that use Numina for the sole purpose of disabling the FOV change, people that don't even use Modular Powersuits. So for the sake of accepting the code into the official builds, it would need a separate config option.

Third, if you are going to assign a new FOV value that doesn't take the first assignment into account, then skip the IAttributeInstance lookup and assignment altogether. Because despite what the naming implies, this event doesn't just fire when the FOV changes, it fires constantly, even when the player is at rest, so there's no need to calculate an unused value. Actually, the value is used, but since it defaults to 1 when unmodified... so my assessment isn't entirely accurate.

kmecpp commented 5 years ago

Ahh that makes more sense. It's not really worth the time for me to make another PR for this just to get the new config option merged into the main 1.7.10 branch and to my knowledge 1.12 MPS doesn't even use Numina so it doesn't really matter to me at this point.

Do you have any news on the original issue?

lehjr commented 5 years ago

I couldn't replicate the original issue. The only thing I can think of it that it's either some heat based damage that isn't accounted for, or some strange server/client sync issue. I tried for over an hour to replicate it but couldn't.