GTNewHorizons / twilightforest

Twilight Forest repository
GNU Lesser General Public License v2.1
14 stars 20 forks source link

Fixed Naga Courtyard component rotations not being loaded. #79

Closed xcube16 closed 2 months ago

xcube16 commented 3 months ago

Related issue: https://github.com/GTNewHorizons/twilightforest/issues/71 Related issue: https://github.com/GTNewHorizons/GT-New-Horizons-Modpack/issues/16353 Previous (fixed the crash, but the structure still gets messed up) PR: https://github.com/GTNewHorizons/twilightforest/pull/73

The previous PR fixed a naga courtyard crash described by the issues above by adding default rotations to the default constructor. However, the correct way to handle this is to load the rotation settings when the NBT is loaded and leave the default constructor empty. This new fix should correctly handle worlds that were left in a crashed state (tested on my survival world that had crashed) and new worlds (untested as of this writing but it is likely good). Old worlds with the old version of the structure should not be affected (untested as of this writing but it should be good).

~TODO: Test completely new worlds~ ~TODO: Test an old world with previously existing TF structures.~

Update: Here is a screenshot from a previously crashing structure generated with the old fix: broken Here is a screenshot from the previously crashing structure generated with the new fix (this PR): fixed

~Update: Other bugs are still present when returning to a half-generated courtyard even in a new world (see below).~ Update: More related bug fixes have been completed. It should be ready to go.

Caedis commented 3 months ago

@Gordon-Frohman as well

Caedis commented 3 months ago

can you run ./gradlew spotlessApply and push the changes?

xcube16 commented 3 months ago

can you run ./gradlew spotlessApply and push the changes?


Task :spotlessJava FAILED
Step 'removeUnusedImports' found problem in 'src/main/java/tconstruct/tools/TFToolEvents.java':
You are running Spotless on JVM 8, which limits you to google-java-format 1.7.
If you upgrade your JVM to 11+, then you can use google-java-format 1.15.0, which may have fixed this problem.
java.lang.Exception: You are running Spotless on JVM 8, which limits you to google-java-format 1.7.
If you upgrade your JVM to 11+, then you can use google-java-format 1.15.0, which may have fixed this problem.
at com.diffplug.spotless.Jvm$Support$1.apply(Jvm.java:181)
at com.diffplug.spotless.FormatterStepImpl$Standard.format(FormatterStepImpl.java:82)
...
... 125 more
Caused by: com.google.googlejavaformat.java.FormatterException: 183:64: error: ')' expected
at com.google.googlejavaformat.java.FormatterException.fromJavacDiagnostics(FormatterException.java:50)
at com.google.googlejavaformat.java.RemoveUnusedImports.parse(RemoveUnusedImports.java:253)
at com.google.googlejavaformat.java.RemoveUnusedImports.removeUnusedImports(RemoveUnusedImports.java:211)
at com.google.googlejavaformat.java.RemoveUnusedImports.removeUnusedImports(RemoveUnusedImports.java:204)
... 129 more

FAILURE: Build failed with an exception.

Odd. I have not made any changes to TFToolEvents.java
Here is my java version:

$ java -version openjdk version "1.8.0_412" ...

Caedis commented 3 months ago

Its optional, otherwise you have to fix it manually.

You can set your Project SDK to use at least java17, preferably java21

Gordon-Frohman commented 3 months ago

Well, technically problem here is not in rotations not loaded, but in structure not generated properly You can see it from the shape of the bushes: those should be more chaotic, but they are not. That's because the generator crashed after the blocks were placed, but before all the rotations were added. That shouldn't be a problem for new sctructures generating, only for those that were generated while generator was crashing

xcube16 commented 2 months ago

Well, technically problem here is not in rotations not loaded, but in structure not generated properly You can see it from the shape of the bushes: those should be more chaotic, but they are not. That's because the generator crashed after the blocks were placed, but before all the rotations were added. That shouldn't be a problem for new sctructures generating, only for those that were generated while generator was crashing

For large minecraft structures that span multiple chunks, not all the chunks generate at once. In fact, the metadata for a structure may be generated from chunks nearby even if part of the structure does not extend that far. If the player then leaves the area before all or even any of the chunks have been generated (and 'populated'; another phase of generation that is offset by 8 blocks requiring 4 chunks), this metadata structure needs to be properly saved and loaded from NBT.

When loading from NBT (unlike the initial creation) the structure components are instantiated via the default (no args) constructor. This was the trigger for the original crash bug many of the fields in ComponentTFNagaCourtyardRotatedAbstract remained null. By implementing func_143011_b and running our initialization from that, we properly fix the crash and don't generate an ugly structure when the player returns to the area to fully generate the structure. Note that coordBaseMode is already loaded from NBT by the superclass so we don't need a custom variable.

As you pointed out, there may be other quirks that previously crashed structures (like the one that I hit on my survival world) will have.

xcube16 commented 2 months ago

I just tested a new world with this PR by setting my render distance to 6 chunks and waiting for one to just barely start to show up on my journeymap. I then saved the world, loaded it back up, and generated the rest of the structure: 2024-07-02_21 58 48 As noted by Gordon, the hedge rows should be more chaotic. This is likely caused by saving/loading bugs in components other than ComponentTFNagaCourtyardRotatedAbstract.

Gordon-Frohman commented 2 months ago

We should probably check if this problem happens while generating other structures as well If that's the case, then the problem is not in courtyard, but in TF/Minecraft itself

xcube16 commented 2 months ago

Here are some other components with variables/state that gets lost on reload: ComponentTFNagaCourtyardHedge#hedgeFloof (not loaded from NBT, defaults to 1.0) ComponentTFNagaCourtyardHedgePadder#hedgeFloof (not loaded from NBT, defaults to 1.0)

I am not sure why the floor tiles are missing (what component generates them?). Maybe there is something in ComponentTFNagaCourtyardMain?

xcube16 commented 2 months ago

We should probably check if this problem happens while generating other structures as well If that's the case, then the problem is not in courtyard, but in TF/Minecraft itself

Minecraft should be fine. It is a problem with the courtyard, but others may also have similar bugs. A good number of other TF structures implement func_143011_b (loading from NBT) when needed but taking a look at newly modified ones might be a good idea. I am not sure if many other structures have been modified as heavily as the naga coartyard (not sure if I want to spoil it as I have not played the pack in a while, he he he).

xcube16 commented 2 months ago

Before reload (just started partly generating): image After reload: image Conclusion: The ComponentTFNagaCourtyardMain#courtyard array is not saved, yet it is still used a bit for placing the 2x2 floor tiles. This may need some minor refactoring to fix.

xcube16 commented 2 months ago

The hedge entropy has been fixed for both half generated structures and new structures. Fixing the path/floor tiles (the 2x2 squares on the ground) was a bit more tricky and required a new NBT tag. As this tag will only be present in completely new structures, half generated ones from before this fix will still be missing some floor paths but that is unavoidable without a serious amount of work (recreating the entire structure metadata from seed, etc).

I feel like this addresses the new issues we found and should be ready. Let me know if anything more needs to be done before merging. :)

Gordon-Frohman commented 2 months ago

Words cannot describe how grateful I am to you for this. Thank you for fixing my mistakes!