Refactor Everything

[unilock]

This PR makes a number of opinionated changes:

• The buildscript has been redone to bring it up to speed with https://github.com/FabricMC/fabric-example-mod
• The dependency on MixinExtras has been removed, since it's not actually used
• The built-in benchmarks have been completely removed, since 100% of users will never see them, they were taking up unnecessary space, and they may be unwanted in external development environments
• The main entrypoint has been removed, since the above change reduced it to simply informational logging, which most users will also never see (the Modrinth page is a better place for this)
• The preLaunch entrypoint has been changed to check the JVM's list of available RandomGenerators at the source, and throw a RuntimeException if it can't find the one it needs
• RandomGeneratorRandom now contains the RandomGeneratorFactory in a static field
• RandomGeneratorRandom#split was made to be more like that of CheckedRandom
• RandomGeneratorRandom now implements its own RandomSplitter (as a private static inner class) instead of using CheckedRandom's, which fixes BetterEnd & BetterNether incompatibilities #34 (upstreamed)
  • ChunkRandomMixin has been removed accordingly (upstreamed)
• RandomGeneratorRandom#nextLong now uses nextLong instead of nextInt, oops (upstreamed)
• RandomMixin#fasterrandom$createLocalInject now uses ThreadLocalRandom like the original Random class does
• The icon has been optimized to make it a tiny bit smaller

[Steveplays28]

Hi, thanks for the PR. Got a few things to note:

• I'm not sure what AnOpenSauceDev thinks, but imo removing the built-in benchmarks isn't a good idea.
• I also think the RandomGeneratorRandom rename is unnecessary. It was named that way to show developers that it uses RandomGenerator under the hood, and implements the Random interface.
• Last but not least, removing the variables (e.x. fabric_loader_version) from the build script makes it harder to maintain, especially if we're going multiloader.

[AnOpenSauceDev]

I'm not sure what AnOpenSauceDev thinks, but imo removing the built-in benchmarks isn't a good idea.

Minecraft could easily switch to LXM if they want, or completely overhaul their random system, so I would like to still be able to easily benchmark this. (Plus, these benchmarks are *tiny*, and only run in developer environments)

I also think the RandomGeneratorRandom rename is unnecessary. It was named that way to show developers that it uses RandomGenerator under the hood, and implements the Random interface.

"TheFasterRandom" might sound cool at first, but it will probably get pretty old over time.

Also, the logger message about Faster Random being present has been removed due to the main entrypoint also being removed. Not sure if it should stay or not.

Aside from that, there isn't really anything I disagree with. I'll go test this out against BetterX when i have the time.

[AnOpenSauceDev]

BetterX (I tested BetterEnd) works without the workaround mixin with this PR, and feature generation (grass, trees, etc) is now 1:1, when it wasn't previously due to nextInt being accidentally used in nextLong.

[Steveplays28]

@AnOpenSauceDev That's really interesting!
Never realized that error, probably copy pasted and forgot to swap it to nextLong(). Awesome that fixing that makes feature generation 1:1.
I can make a PR to fix that real quick, and move some classes into their own packages (just to clean up the main package a bit).

By the way, all PR branches I make are protected and can't be deleted after they're merged. Would you mind either changing protection rules to only protect the main branch, or deleting merged branches?

[unilock]

I would like to still be able to easily benchmark this.

I agree, but I still don't think embedding the benchmarks in the same jar used in production is a good idea - in case e.g. a developer is testing compatibility between their mod and Faster Random in their own dev environment.

Maybe a testmod could be used instead? Fabric API uses quite a few of those: https://github.com/FabricMC/fabric/blob/fc5276a561510a620bb5793b63b77248a59d566d/build.gradle#L172-L215

I also think the RandomGeneratorRandom rename is unnecessary. It was named that way to show developers that it uses RandomGenerator under the hood, and implements the Random interface.

I think it would probably be better to name it something like L64X128MixRandom in that case? That's what it really uses under the hood; RandomGenerator is just a shim of sorts.

[Steveplays28]

Test mod sounds like a good idea, could look into that!

I think it would probably be better to name it something like L64X128MixRandom in that case? That's what it really uses under the hood; RandomGenerator is just a shim of sorts.

Maybe in the future we'd want a config for users so keeping it the same name as currently is my vote, even if RandomGenerator is just a shim.

[AnOpenSauceDev]

This wouldn't execute all at once. 99% of the time in Java, the CPU goes through a function linearly unless you directly create a new non-blocking thread. All this should do is generate a number, make the JVM discard the result, then generate the next number. The main thread is blocked by this, so the worker threads probably aren't doing much, let alone on the same core.

If you're talking about individually timing each random function, that's also not necessary, as ideally you only want to keep one generator that's the best all-round so you don't have to waste time trying to make multiple generators work nicely together.

[davidromrell]

ok

[AnOpenSauceDev]

Is this PR still being worked on? If not, I could try fixing parts of this up for a merge.

[unilock]

@Steveplays28

Maybe in the future we'd want a config for users so keeping it the same name as currently is my vote, even if RandomGenerator is just a shim.

That's a good point! I'll change the name back then.

@AnOpenSauceDev

Is this PR still being worked on?

Sorry, I recently got a new laptop, and haven't transferred everything over from the old one yet (including my dev environment). I'll rebase it on the current master branch soon.

[unilock]

This branch is now up-to-date with upstream Faster Random 4.0.1. I've also moved the benchmarks to a testmod; they can be run with the runTestmod Gradle task.

[Steveplays28]

Awesome. Could you check up my review comment?
Still of the opinion that template expansion for gradle.properties is nice to have and makes it easier to maintain the mod.

[unilock]

@Steveplays28

Could you check up my review comment?

What review comment...?

Still of the opinion that template expansion for gradle.properties is nice to have and makes it easier to maintain the mod.

The only template expansion I removed was for loader_version, which I think should be kept static - Faster Random doesn't strictly depend on any features added in newer versions of Fabric Loader, not even Mixin Extras.

Also, Quilt Loader doesn't always provide the latest version of Fabric Loader - so if Faster Random declares a dependency on a version of Fabric Loader that Quilt hasn't caught up to yet, it'll refuse to start in Quilt environments for no good reason.

The version I have it set to, >=0.14, should be adequate in most cases. It could probably even be set to *, but I haven't tested it on Fabric Loader versions below 0.14... not that they're used (or should be used!) anymore, anyway.

[AnOpenSauceDev]

LGTM, aside from my review question about the PreLaunch changes.