Occultism

Occultism

19M Downloads

`MultiChunkFeature` unsafely casts `Random` to `WorldGenRandom`

alcatrazEscapee opened this issue ยท 1 comments

commented

Describe the bug

MultiChunkFeature does an unsafe cast here:

this.getRootPositions(context.level(), context.chunkGenerator(), (WorldgenRandom) context.random(), generatingChunk, context.config());

To WorldGenRandom, when the type of FeaturePlaceContext.random() is only specified as RandomSource. This is unsafe, and mods which do not use WorldGenRandom will cause this to crash.

To Reproduce

  1. Install TerraFirmaCraft 1.18.2-2.2.27 and Occultism 1.18.2-1.81.4
  2. Try and create a ne world
  3. Crash

Example crash log (from TerraFirmaCraft/TerraFirmaCraft#2486): crash-report.txt

System:

  • Occultism Version: 1.81.4
  • OS: Mac OS X (x86_64) version 11.7.7
  • Minecraft Version: 1.18.2, although the problematic code is present in 1.20.1 (latest) as well
  • Modpack Link and Version, or list of mods: see crash report, although only the aforementioned two mods are necessary to reproduce.

Additional context

TerraFirmaCraft does not use WorldGenRandom example as it is slowly being replaced throughout Mojang's code, and is the legacy version of the new random implementation (XoroshiroRandomSource). It also has some terrible seeding in setFeatureSeed which has caused us (and vanilla) issues with it's low entropy in feature seeding before.

In this instance, there's no need to use WorldGenRandom in this code. You could accomplish the same thing by simply creating a new Random() or new XoroshiroRandomSource(), and calling .setSeed() with a hash function of your own making (or even copying the hash function in WorldGenRandom.setLargeFeatureSeedWithSalt, because that one at least, is passible.

There is also another problem in the current implementation: you reset the seed of the random which is provided to every feature in sequence. This means that any feature that runs after your feature gets affected by the reset seed instead of being purely chunk based. So instead, you should really be creating a new random anyway, if you intend to call .setSeed() on it directly.

commented

thanks for the report, I had no idea!
Worldgen is not my strength so the detailed fix steps are much appreciated