Add a registration system for spreadable block compatibility. #3383

gniftygnome · 2023-10-19T00:10:52Z

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:

Vanilla grass can spread to modded dirt and create the correct modded grass.
Modded grass can spread to vanilla dirt or other modded dirt and create the correct vanilla or modded grass.
The same goes for mycelium.
Additional registries can be easily created for new blocks which extend SpreadableBlock.

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.

modmuss50 · 2023-10-20T21:22:07Z

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

+	 * @param type The registry type Identifier for the desired spreadable block registry
+	 * @return The SpreadableBlockRegistry for the given ID
+	 */
+	static SpreadableBlockRegistry getOrCreateInstance(Identifier type) {


Maybe just get here? Im not sure the OrCreateInstance is really that useful to the API user.

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.

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

...-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java

modmuss50 · 2023-10-20T21:26:47Z

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

+	/**
+	 * Gets the spreadable block state (if any) for a given bare block state.
+	 *
+	 * @param bareBlockState The bare block state to search the registry for


Just reading this is not super clear to me what The bare block state means? Would this be dirt as opposed to grass?

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.

In 11a0801 I've reworded a bunch of the API javadocs. Hopefully this is better now, but let me know.

kevinthegreat1 · 2023-11-09T19:51:16Z

...-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java

+public final class SpreadableBlockRegistryImpl implements SpreadableBlockRegistry {
+	private static final Logger LOGGER = LoggerFactory.getLogger(SpreadableBlockRegistryImpl.class);
+
+	private static final Map<Block, Identifier> SPREADABLE_TYPE;


This seems to only allow a block to be spreadable. What if I only want one block state of a block to be spreadable?

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.

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.

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.

Juuxel

Mostly javadoc notes

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

Juuxel · 2023-12-28T00:30:55Z

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

+	 * @param type The registry type Identifier for the desired spreadable block registry
+	 * @return The SpreadableBlockRegistry for the given ID
+	 */
+	static SpreadableBlockRegistry getOrCreateInstance(Identifier type) {


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

...-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

Juuxel · 2023-12-28T00:41:23Z

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

+
+	/**
+	 * 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.


Doesn't SpreadableBlockRegistry.add map all blocks of the type to registry, not just a "primary" block?

fabric/fabric-content-registries-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java

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);

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).

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)

Juuxel · 2023-12-29T14:43:05Z

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

+	 *
+	 * @return An immutable copy of the spreadable block types map
+	 */
+	static ImmutableMap<Block, Identifier> getSpreadableTypes() {


Is there any reason to use indirect Identifiers as the values instead of the registry instance directly?

I wonder if a RegistryKey is the soltuion here? or maybe leave that until we are forced into it?

It's the id of a SpreadableBlockRegistry, not a vanilla registry entry like Block. In other words, just a key of this map:

fabric/fabric-content-registries-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java

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.

Ah yeah, sorry I didnt look at the full context. I think this would be a good idea to change.

I concur with the other comments on this, should probably map to registry instances directly.

Juuxel · 2023-12-29T14:44:53Z

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

+
+	/**
+	 * 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.


Right. I was just wondering if it's really the "primary" spreadable block if it applies to all spreadable blocks using the registry.

modmuss50 · 2024-01-05T14:48:07Z

...-v0/src/main/java/net/fabricmc/fabric/impl/content/registry/SpreadableBlockRegistryImpl.java

+	private static final Map<Block, Identifier> SPREADABLE_TYPE = new IdentityHashMap<>();
+	private static final Map<Identifier, SpreadableBlockRegistry> REGISTRIES = new HashMap<>();


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?

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.

modmuss50 · 2024-01-05T14:53:50Z

Going to keep this one in last call for a little longer, I think there are still some design questions to be clarified.

gniftygnome · 2024-01-05T20:47:23Z

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).

modmuss50 · 2024-01-05T20:58:57Z

Yes, I am still not quite to a good point to pick this up again

No worries at all, Im quite happy to get this to the finish line myself, its 99% of the way there.

Syst3ms · 2024-01-15T11:32:32Z

...stries-v0/src/main/java/net/fabricmc/fabric/mixin/content/registry/SpreadableBlockMixin.java

+					target = "Lnet/minecraft/util/math/BlockPos;add(III)Lnet/minecraft/util/math/BlockPos;",
+					shift = At.Shift.BY, by = 2
+			),
+			locals = LocalCapture.CAPTURE_FAILHARD


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?

Syst3ms · 2024-01-15T11:33:03Z

...nt-registries-v0/src/main/java/net/fabricmc/fabric/api/registry/SpreadableBlockRegistry.java

+	 *
+	 * @return An immutable copy of the spreadable block types map
+	 */
+	static ImmutableMap<Block, Identifier> getSpreadableTypes() {


I concur with the other comments on this, should probably map to registry instances directly.

gniftygnome added 3 commits October 17, 2023 22:20

Add a registration system for spreadable block compatibility.

18d6da2

Revert IDEA moving things around unnecessarily

095bb81

Code style compliance and add tests.

486ee06

modmuss50 reviewed Oct 20, 2023

View reviewed changes

Address some review requests.

11a0801

modmuss50 requested a review from a team November 5, 2023 19:26

Add an accessor for an immutable copy of SPREADABLE_TYPE.

725e336

Juuxel added enhancement New feature or request priority:low Low priority PRs that don't immediately need to be resolved labels Nov 6, 2023

kevinthegreat1 reviewed Nov 9, 2023

View reviewed changes

modmuss50 approved these changes Dec 12, 2023

View reviewed changes

modmuss50 requested a review from a team December 12, 2023 11:12

modmuss50 added the last call If you care, make yourself heard right away! label Dec 27, 2023

Juuxel suggested changes Dec 28, 2023

View reviewed changes

More requested changes from review.

fee1156

- Various javadoc changes - SpreadableBlockRegistry.getOrCreateInstance is now getInstance - When in Rome, do as the Romans say to (or befriend a centurion)

Juuxel reviewed Dec 29, 2023

View reviewed changes

modmuss50 reviewed Jan 5, 2024

View reviewed changes

Syst3ms suggested changes Jan 15, 2024

View reviewed changes

modmuss50 removed the last call If you care, make yourself heard right away! label Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a registration system for spreadable block compatibility. #3383

Add a registration system for spreadable block compatibility. #3383

gniftygnome commented Oct 19, 2023

modmuss50 Oct 20, 2023

gniftygnome Oct 20, 2023

Juuxel Dec 28, 2023

modmuss50 Oct 20, 2023

gniftygnome Oct 20, 2023

gniftygnome Oct 21, 2023

kevinthegreat1 Nov 9, 2023

gniftygnome Nov 9, 2023

kevinthegreat1 Nov 10, 2023

gniftygnome Nov 10, 2023

Juuxel left a comment

Juuxel Dec 28, 2023

Juuxel Dec 28, 2023

gniftygnome Dec 28, 2023

Juuxel Dec 29, 2023

Juuxel Dec 29, 2023

modmuss50 Jan 2, 2024

Juuxel Jan 2, 2024

modmuss50 Jan 4, 2024

Syst3ms Jan 15, 2024 •

edited

Juuxel Dec 29, 2023

modmuss50 Jan 5, 2024

gniftygnome Jan 5, 2024

modmuss50 commented Jan 5, 2024

gniftygnome commented Jan 5, 2024

modmuss50 commented Jan 5, 2024

Syst3ms Jan 15, 2024 •

edited

Syst3ms Jan 15, 2024 •

edited

	@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);

		private static final Map<Block, Identifier> SPREADABLE_TYPE = new IdentityHashMap<>();
		private static final Map<Identifier, SpreadableBlockRegistry> REGISTRIES = new HashMap<>();

Add a registration system for spreadable block compatibility. #3383

Are you sure you want to change the base?

Add a registration system for spreadable block compatibility. #3383

Conversation

gniftygnome commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juuxel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Syst3ms Jan 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modmuss50 commented Jan 5, 2024

gniftygnome commented Jan 5, 2024

modmuss50 commented Jan 5, 2024

Syst3ms Jan 15, 2024 • edited

Choose a reason for hiding this comment

Syst3ms Jan 15, 2024 • edited

Choose a reason for hiding this comment

Syst3ms Jan 15, 2024 •

edited

Syst3ms Jan 15, 2024 •

edited

Syst3ms Jan 15, 2024 •

edited