-
Notifications
You must be signed in to change notification settings - Fork 405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add nether biomes API #496
Conversation
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/api/biomes/v1/NetherBiomes.java
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/impl/biome/NetherBiomesImpl.java
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/mixin/biome/MixinTheNetherDimension.java
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/mixin/biome/MixinTheNetherDimension.java
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/impl/biome/NetherBiomesImpl.java
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/api/biomes/v1/NetherBiomes.java
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/mixin/biome/MixinTheNetherDimension.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some easy fixes.
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/api/biomes/v1/NetherBiomes.java
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/api/biomes/v1/NetherBiomes.java
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/impl/biome/NetherBiomesImpl.java
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/mixin/biome/MixinTheNetherDimension.java
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/resources/fabric-biomes-v1.mixins.json
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/impl/biome/NetherBiomesImpl.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: coderbot16 <coderbot16@gmail.com>
import org.spongepowered.asm.mixin.injection.ModifyArg; | ||
import org.spongepowered.asm.mixin.injection.Redirect; | ||
|
||
import net.minecraft.class_4767; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final review, I promise! I think we should just clarify to API users what settings they need to tweak.
private NetherBiomes() { } | ||
|
||
/** | ||
* Adds a biome to the Nether generator. Biomes must set their own noise generation values in the biome settings class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps elaborate on which settings and have a {@link Biome.Settings}
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to detail that you need to set up the biome noise in settings properly or the biomes have literally zero chance to generate in the nether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i will add some basic information and then write some detailed explanation on the wiki
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build passes. Style is good.
@Earthcomputer can we get a second review from you? Just want to clean up the reviews list :p |
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/api/biomes/v1/NetherBiomes.java
Outdated
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/api/biomes/v1/NetherBiomes.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: coderbot16 <coderbot16@gmail.com> Co-Authored-By: Juuxel <6596629+Juuxel@users.noreply.github.com>
I think this is ready for a final review from Player/modmuss. Can @Juuxel give this another look? |
Almost forgot, we should probably increment the version number to 0.2.0. I'm not sure how this will interact with #491, or if we'll need to bump it to 0.3.0 in the other PR. |
Bump to 0.2.0 in both and unless they're merged at the same time, bump the one that was merged last to 0.3.0 |
/** | ||
* API that exposes the internals of Minecraft's nether biome code. | ||
*/ | ||
public class NetherBiomes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everywhere else in API (afaik) this class would be an interface with an INSTANCE field that NetherBiomesImpl implements (see e.g. net.fabricmc.fabric.api.client.model.ModelLoadingRegistry.) Why is this different here?
Not that it makes any real difference, but it would be more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to be consistent with the rest of the biome API which follows the same pattern that I used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yeah I didn't even notice we did the OverworldBiomes one any differently. Also I noticed in OverworldBiomes we had the class as final
, maybe put that here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if this style exists somewhere previously in API, then fair enough.
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/api/biomes/v1/NetherBiomes.java
Show resolved
Hide resolved
fabric-biomes-v1/src/main/java/net/fabricmc/fabric/impl/biome/NetherBiomesImpl.java
Outdated
Show resolved
Hide resolved
/** | ||
* Sets for internal use only! Stores data that is used by the Nether dimension mixin. | ||
*/ | ||
public final class NetherBiomesImpl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make this a class and not add it to InternalBiomeData
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great question if i'm being honest
Module also needs to be marked to only run on 1.16+ versions |
add 1.16 dependency, merge NetherBiomesImpl into InternalBiomeData, fix code style, and fix mixin target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement #499 and then I think this is good to merge.
* Add nether biomes API * Update fabric-biomes-v1.mixins.json * fixed license * smarter injection point * Apply suggestions from coderbot's review Co-Authored-By: coderbot16 <coderbot16@gmail.com> * fix code style * remove redundant import * improve docs and update yarn version * Apply suggestions from code review Co-Authored-By: coderbot16 <coderbot16@gmail.com> Co-Authored-By: Juuxel <6596629+Juuxel@users.noreply.github.com> * bump version * mark classes as final * Changes from modmuss's review add 1.16 dependency, merge NetherBiomesImpl into InternalBiomeData, fix code style, and fix mixin target * Update build.gradle * Update API to inform users that it is experimental * remove beta annotion * optimize imports Co-authored-by: coderbot16 <coderbot16@gmail.com> Co-authored-by: Juuxel <6596629+Juuxel@users.noreply.github.com>
Add a new class and mixin to allow modders to inject their own biomes into the nether in a compatible manner.