sinkillerj / ProjectE

ProjectE. A complete rewrite of EE2 for modern Minecraft versions.
MIT License
405 stars 208 forks source link

EMC going to negative #1887

Closed HSavageheart closed 4 years ago

HSavageheart commented 5 years ago

Tickets that do not conform to this template will be closed without comment

Exact ProjectE version (do not say "latest", "latest on Curse", or similar): 1.12.2-PE1.4.1 Exact Forge version: 14.23.5.2836 Link to crash log (please use a paste site, do not attach the .txt or paste the log inline): No crash Steps to reproduce: 1. using Project EX 1.2.0.35 to generate EMC really fast and after a while it goes to negative. maybe after a specific amount?

What I expected to happen: expected to have my EMC Value really high What happened instead: EMC went to negative meaning i lost all my EMC http://prntscr.com/nq8muc

HSavageheart commented 5 years ago

found out its cause i am at a max value of 9.2 quintillion it changes it to -9.2 Quintillion and counts further to 0 emc where it resets

sinkillerj commented 5 years ago

This is probably a Project EX issue, but if its not ill look into it.

HSavageheart commented 5 years ago

this has been told to the person from project ex and hes telling us to contact you about it.

pupnewfster commented 5 years ago

Honestly as ProjectE has had things to ensure it doesn't go past the bounds, and Project EX added ways around that it is not really ProjectE's fault that it doesn't load it properly when it makes certain assumptions. It might make sense for ProjectE to just reset invalid data to 0 EMC, but in my opinion given Project EX is the only way it would go past the bound, they should really just add a data fixer to set you to zero or whatever value they want to fix it to.

TheBlueCrystal commented 5 years ago

Hello everyone, perhaps I may suggest something? Would it by any chance be possible to use the builtin java.math.BigInteger datatype (only internally or also in the API) instead of long? BigInteger can represent arbitrary numbers and is completely supported by all math-operations. It can be primed by long- und string-representations of numbers (an can of course also be exported to those). This datatype exists since Java 1.2 and is part of the native java.math library. As far as I know EMC calculations don't occur hundred-thousands or millions of times a second, so the (very small) performance impact should not be noticeable at all. And this way the EMC value would have no practically reachable maximum value.

MaPePeR commented 5 years ago

I think we have a bet open somewhere, that as soon as we switch to long someone is going to ask for BigInt. 🤷‍♂ 😅

pupnewfster commented 5 years ago

IIRC The biggest issues with BigInt is that minecraft NBT and packet sending has no native support for them so a wrapper would have to be made. But either way a lot of it had to do with getting everything stable first as a long. (Initially the change made item EMC become a long instead of an int, and the latest change (because all the other spots were capped at int/long from checks anyways) changed internally saving emc for the remainder of things from a double to a long so that there is no precision loss at larger numbers.

TheBlueCrystal commented 5 years ago

Yes of course, minecraft NBT has no native support for BigInteger, but I guess the byte-array tag-type (type 7) would be a great way to store the internal 2's complement byte-representation of the BigInteger (it can export into byte-array directly from the class). That should suffice for EMC-transport, don't you think? This would solve the precission-problem and the max-value problem in one sweep. And yes the mantissa of double is a bit short for mods like Project-EX :smiley:

Airtheon commented 5 years ago

I have also experienced this issue using Project E with Project EX. I was first going to suggest using unsigned longs, to avoid the overhead of bigInt, but reading the above comments I believe bigInt to be the best solution as they do solve both those problems.

TheBlueCrystal commented 5 years ago

@pupnewfster, I'm sorry that I did not go into all detail about your response in my last message. You are of course right, that this issue arises almost only using additional mods like Project-EX - even if it's theoretically possible to reach that cap with ProjectE (and for example some tick-multiplier mods like "Torcherino" or "Not enough wands" or others - perhaps even in combination with the "watch of flowing time" (if enabled in the config to also affect collectors)) alone. This way you could get million of "ticks per tick" and produce considerable amounts of EMC. Of course this would be horrible for the server (and also client-) performance, but it's theoretically possible to do such things. Resetting the (invalid / negative) EMC value to 0 is in my opinion the worst thing to do - since this way, you would loose all your EMC. Consider a very extreme example: You have EMC points of magnitude of the exact maximum positive value of long. If you put a single cobble stone into your transmutation table(t), you would loose all your EMC since it would flip the sign of the long variable and subsequently get "corrected" to 0. That's not something a user would expect to happen - especially if he/she is unaware of maximum sizes and mechanics of computer/java data types.

The other possibility - to just cap the value at it's maximum is of course possible, but also not a desireable solution in my opinion, since I guess most users of minecraft are not aware of such "arbitrary" limits of data types or as consequence of their EMC value. (In the former example the cobble stone would just be lost - not a big loss, granted, but also not something one would expect to happen). Also if they set up some collector array and go mining / building all day just to come back and realize that their EMC value has reached it's maximum five minutes after they left and did not change any further would be a lot frustrating (at least to me). That's the reason why I suggested BigInteger in the first place, since it would solve the problem(s) once and for all and the game would behave as expected by the player.

pupnewfster commented 5 years ago

I agree BigInteger would be the ideal solution (other than object overhead, and the other various things of Minecraft not having a built in way to handle it). The bigger issue is that ProjectE's code already limited things to int/long even though in places it stored them as a double. (So even if you had various tick accelerators you would not be able to go past the limit). When I initially updated the PR converting it internally from double (and bounded to int/long) to just storing the stuff as a long Project EX if I remember did not add a way to bypass the bound checks in place.

But I do also agree that technically setting it to zero if it is invalid would not be ideal, however as ProjectE has had bounds in place for how high it could go, there is no way for it to know what the actual value should be if it is out of those bounds. (Which is why I would assume the logical default on error would be zero). That is also why I said and still believe that it really should be on Project EX to add a datafixer for when a player joins to set it to some proper value/handle it as Project EX is what bypassed the limits and is the only mod that can truly properly determine what value it should default to.

I know when Latvian noticed the PR he was mentioning that player EMC should be a BigInteger, which I agreed with; however, I still stand by the fact that doing that change in addition to the change to longs would have made the PR more of a mess and harder to verify nothing got broken. The bigger thing is that since then a decent amount of time passed before the PR was merged, and I have started working on other projects and also am unsure of the best way of doing some of the things about BigInteger anyways. (I have suggested to him multiple times that he just makes a PR given from how he talked he seemed to have a decent idea of how it would have to be implemented, but he never seemed interested in actually making it a reality himself.)

Airtheon commented 5 years ago

I'm quite inexperienced when it come to modding, so this might be unfeasible, but would it not make more sense to give emc it's own datatype?

That way when a change is needed only the internals of the data type need to be changed and all other code can remain the same.

Then internally this could be represented in anyway one likes and possibly even be overwritten by mods like Project EX.

LatvianModder commented 5 years ago

I will add protection in Project EX for it not to go over limit on my side, when EMC is generated, but that bug has to be handled by ProjectE on loading, because there is no way I can do that. Its a simple if(emc < 0L) emc = Long.MAX_VALUE; type of thing where data read happens (because the only way your EMC can be negative, is when it overflows max long, which means you already had more than 9E EMC so its not gonna give random players a ton of EMC). I also suggested BigInteger for personal EMC, but the PR was already made. If its used only in once place, it adds like.. couple bytes to RAM, so its no overhead at all

HSavageheart commented 5 years ago

i wish that i could help more since i followed almost 1 year of C#. i am saying almost cause i was forced to stop due to medical problems. but it makes me happy that this post has had so many people talking about a fix and it makes me happier that both mod makers are on the case :D

pupnewfster commented 4 years ago

ProjectE for 1.14+ uses a BigInteger for storing player emc.