MrTJP / ProjectRed

Redstone Engineering
MIT License
466 stars 178 forks source link

Bus Converter bugfix #1759

Closed EnronEvolved closed 1 year ago

EnronEvolved commented 1 year ago

As promised, I've got a pull request containing the fix for that bug I found (#1758).

I don't know if you're planning on using any of the remaining Scala code, but I've also cleaned out all of the procedure syntax from that portion of the codebase (manually, I might add; in hindsight I probably should have passed the migration options to the compiler). Now the command line isn't saturated with deprecation warnings!

MrTJP commented 1 year ago

Thanks for looking into this. Have a few comments:

Firstly, like you mentioned in the issue, this wouldn't fix devices already "stuck". We should detect older tags and do the following:

  1. If an old tag is detected (i.e. if "in0" and "out0" are saved as bytes), set 2 new forceBInUpdate and forceBOutUpdate boolean flags true.
  2. Once an external block update happens, the gate's gateLogicOnChange() method will fire. On line 400 there is a check to see if bIn changed. Force that check to be true if forceBInUpdate is set. This will force a tick to be scheduled for the change. Unset the forceBInUpdate.
  3. On the scheduled tick, method gateLogicOnScheduledTick() will fire. Similarly, force the bOut change check on line 414 to be true based on the forceBOutUpdate boolean and unset the flag.
  4. Leave comments on those 2 boolean flags bc someone looking at that without this issue's context will be very confused about why they exist.

This is just at the top of my head, feel free to explore other ways to do this.

Secondly, I know it was annoying to scrub the procedure syntax from the code base, and I appreciate your efforts. I am unfortunately going to have to ask you to revert those commits, because I am in the middle of a large rewrite for Integration and Transmission to convert them to Java and port to 1.18+. Sorry about that!

EnronEvolved commented 1 year ago

I've twiddled branches around so those procedure syntax commits are no longer on the branch.

Unsure how to implement that migration solution myself, because I don't know enough about how CompoundNBT works to detect types before the data is fetched. It seemed to fetch data of the appropriate type happily enough with getShort when I first ran a world with the patch. God knows where it got the upper byte of the short from that first time...

(As an aside, I asked about Scorge on the MCForge forum. I don't know if the apparent lack of updates there is prompting the Java port, but if so, seems like the Forge team forgot to release Scorge on CurseForge for a year and a half)

MrTJP commented 1 year ago

I think there are "contains" methods on CompoundNBT, unsure if they take into account the type. If not, then likely its just reading garbage for the upper byte once you move from byte to short. In that case, you can always save a new value into the tag, set to a non-zero value (something like byte tagVersion = 2). Has to be non-zero because trying to read values that don't exist will return a default (0 for byte/short/int, "" for string, 0.0 for floats, etc).

MrTJP commented 1 year ago

Awesome. Will test it out asap and merge it in.