-
Notifications
You must be signed in to change notification settings - Fork 399
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 a registration system for spreadable block compatibility. #3383
base: 1.20.2
Are you sure you want to change the base?
Conversation
* @param type The registry type Identifier for the desired spreadable block registry | ||
* @return The SpreadableBlockRegistry for the given ID | ||
*/ | ||
static SpreadableBlockRegistry getOrCreateInstance(Identifier type) { |
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.
Maybe just get
here? Im not sure the OrCreateInstance
is really that useful to the API user.
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.
It creates so the API user can add a custom SpreadableBlock type. While I don't have a specific need for this myself at the moment, I can imagine a lot of potential uses. The test cases I provided include creating a spreadable version of Podzol that spreads to Coarse Dirt as an example.
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 name does seem too long considering there's no get
that doesn't have the "or create" functionality.
...stries-v0/src/main/java/net/fabricmc/fabric/mixin/content/registry/SpreadableBlockMixin.java
Outdated
Show resolved
Hide resolved
...stries-v0/src/main/java/net/fabricmc/fabric/mixin/content/registry/SpreadableBlockMixin.java
Show resolved
Hide resolved
...-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java
Outdated
Show resolved
Hide resolved
/** | ||
* Gets the spreadable block state (if any) for a given bare block state. | ||
* | ||
* @param bareBlockState The bare block state to search the registry for |
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.
Just reading this is not super clear to me what The bare block state
means? Would this be dirt as opposed to grass?
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.
It's Dirt for the two vanilla registries. It could be anything for any modded registries. I'll think about how to clarify that a bit.
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.
In 11a0801 I've reworded a bunch of the API javadocs. Hopefully this is better now, but let me know.
public final class SpreadableBlockRegistryImpl implements SpreadableBlockRegistry { | ||
private static final Logger LOGGER = LoggerFactory.getLogger(SpreadableBlockRegistryImpl.class); | ||
|
||
private static final Map<Block, Identifier> SPREADABLE_TYPE; |
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.
This seems to only allow a block to be spreadable. What if I only want one block state of a block to be spreadable?
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 was told to keep it really simple, so I did. Then I was told it has to cover a whole bunch more use cases (and here's another)... Effectively at least for now this is pretty much a dead PR. I intend to support all this stuff over in Terraform API where there aren't so many constraints on the implementation and I can already use MixinExtras.
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 understand your frustration, however, once you add something to FAPI, you basically can't make a breaking change at all to ensure compatibility. Therefore, a new API often goes through a lot of deliberation and review. Also, I'm just asking a question about something I noticed.
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'm not frustrated, though. I have an alternative avenue for this. The only way it impacts me is other mod authors will likely use incompatible mixins because FAPI doesn't provide something. I don't see this as a case of "diligence" on FAPI's end though. It's more like a lack of consensus or leadership on what is acceptable for inclusion. If I had written this just to get it into FAPI then it would be frustrating, but I had to do the work anyway for Terraform API.
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.
Mostly javadoc notes
...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java
Outdated
Show resolved
Hide resolved
...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java
Outdated
Show resolved
Hide resolved
* @param type The registry type Identifier for the desired spreadable block registry | ||
* @return The SpreadableBlockRegistry for the given ID | ||
*/ | ||
static SpreadableBlockRegistry getOrCreateInstance(Identifier type) { |
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 name does seem too long considering there's no get
that doesn't have the "or create" functionality.
...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java
Outdated
Show resolved
Hide resolved
...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java
Outdated
Show resolved
Hide resolved
...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java
Outdated
Show resolved
Hide resolved
...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java
Outdated
Show resolved
Hide resolved
...-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java
Outdated
Show resolved
Hide resolved
...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java
Outdated
Show resolved
Hide resolved
|
||
/** | ||
* Gets an immutable copy of the spreadable block types map, which maps the | ||
* canonical or "primary" block of the type to the registry for the type. |
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.
Doesn't SpreadableBlockRegistry.add
map all blocks of the type to registry, not just a "primary" block?
Lines 97 to 102 in 725e336
@Override | |
public void add(Block bareBlock, BlockState spreadBlock) { | |
Objects.requireNonNull(bareBlock, "bare block cannot be null"); | |
Objects.requireNonNull(spreadBlock, "spread block cannot be null"); | |
Identifier oldRegistryId = SPREADABLE_TYPE.put(spreadBlock.getBlock(), this.registryId); |
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.
Yes, SpreadableBlockRegistry.add
is called to add all the spreadable block transitions to the associated registries. The types map is basically a map of registry key (which is the primary spreadable block) to registry. The map is used by the vanilla mixin to implement vanilla-to-mod spread. Spread from a modded block must be implemented in the block.
You could argue the API implementation is not complete without a FabricSpreadableBlock
type thing with a default implementation of the mod->any logic. I wouldn't argue against it; this PR probably should be rewritten anyway, now that MixinExtras is available in the API. When I have some time in January, I'd like to take another pass at trying to implement some of the more complicated requests (such as allowing mods to set the new block state).
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.
Right. I was just wondering if it's really the "primary" spreadable block if it applies to all spreadable blocks using the registry.
- Various javadoc changes - SpreadableBlockRegistry.getOrCreateInstance is now getInstance - When in Rome, do as the Romans say to (or befriend a centurion)
* | ||
* @return An immutable copy of the spreadable block types map | ||
*/ | ||
static ImmutableMap<Block, Identifier> getSpreadableTypes() { |
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.
Is there any reason to use indirect Identifier
s as the values instead of the registry instance directly?
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 wonder if a RegistryKey is the soltuion here? or maybe leave that until we are forced into it?
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.
It's the id of a SpreadableBlockRegistry, not a vanilla registry entry like Block. In other words, just a key of this map:
Line 41 in fee1156
private static final Map<Identifier, SpreadableBlockRegistry> REGISTRIES = new HashMap<>(); |
The instances for each id are constant once they're created (and the spreadable block registries that are in the map have been created), so it'd be simpler to just have the registry itself in the map IMO.
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.
Ah yeah, sorry I didnt look at the full context. I think this would be a good idea to change.
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 concur with the other comments on this, should probably map to registry instances directly.
|
||
/** | ||
* Gets an immutable copy of the spreadable block types map, which maps the | ||
* canonical or "primary" block of the type to the registry for the type. |
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.
Right. I was just wondering if it's really the "primary" spreadable block if it applies to all spreadable blocks using the registry.
private static final Map<Block, Identifier> SPREADABLE_TYPE = new IdentityHashMap<>(); | ||
private static final Map<Identifier, SpreadableBlockRegistry> REGISTRIES = new HashMap<>(); |
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.
is there ever a case where they may be multiple SpreadableBlockRegistry
for a given Block
. I was looking at fixing it up following Juzz's comments and it seems there might not be. Unless im missing something this could just be a single Map<Block, SpreadableBlockRegistry>, without having any sort of ID at all?
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.
It took me a while to remember why I did that. :) If mod A adds a spreadable block and mod B optionally extends A, I thought it would be nice for mod B to be able to get the registry for that spreadable block without having to import mod A (and guard the reference) to get the block itself. Using an identifier makes it possible to just create the required identifier in mod B when fetching the registry.
Going to keep this one in last call for a little longer, I think there are still some design questions to be clarified. |
Yes, I am still not quite to a good point to pick this up again (and as I mentioned, it could probably be improved now that MixinExtras is available, anyway). |
No worries at all, Im quite happy to get this to the finish line myself, its 99% of the way there. |
target = "Lnet/minecraft/util/math/BlockPos;add(III)Lnet/minecraft/util/math/BlockPos;", | ||
shift = At.Shift.BY, by = 2 | ||
), | ||
locals = LocalCapture.CAPTURE_FAILHARD |
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.
As you mentioned yourself, @Local
time. That being said, shifts and conditionals are always a bit weird, have you checked that the output is what we want?
* | ||
* @return An immutable copy of the spreadable block types map | ||
*/ | ||
static ImmutableMap<Block, Identifier> getSpreadableTypes() { |
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 concur with the other comments on this, should probably map to registry instances directly.
This PR adds a registration system for spreadable blocks (blocks which extend SpreadableBlock). Each "type" of spreadable block has its own registry instance, which maps a bare block (f.e. Dirt) to a spreadable block (f.e. Grass or Mycelium). Two instances are provided for mods extending vanilla grass and mycelium. Additional registries can be requested for new modded spreadable types. I believe this is the simplest implementation appropriate for FAPI which implements enough features to be useful and is minimally intrusive. For example, this provides enough functionality I expect to be able to use it in Terraform API.
Goals:
This PR does not address decay of spreadable blocks, because that can be easily handled by mods via mixin.
This PR also does not address allowing mods to change the conditions for spread (such as minimum and maximum light levels), although I could imagine this registry optionally storing a reference to an alternative canSpread method to be used in lieu of vanilla's call to the static method in SpreadableBlock (if only they used an instance method instead ... sigh).
I have added tests to the test mod and tested using runTestmodClient, and also by modifying Terrestria to use the API in production.