Add textile-utils-api-v1 #3

PhoenixVX · 2020-11-30T01:43:04Z

Bikeshed:tm: on new branch

BoogieMonster1O1

The id should definitely not be hardcoded
Registering blocks and items imo should be in registry sync as it would involve remapping the registry to match ids

BoogieMonster1O1 · 2020-11-30T01:50:06Z

textile-utils-api-v1/src/main/resources/textile-utils-api-v1.mixins.json

+  "package": "net.textilemc.textile.mixin.recipe",
+  "compatibilityLevel": "JAVA_8",
+  "mixins": [
+    "RecipeDispatcherMixin"


Why is this here?

Oops... left over from copying that module. Will fix.

About the ID, would you figure out what is needed for that?

What do you mean about id? if you mean where it says id-256, that is what vanilla uses in the blockItem registry. See the bottom of the static constructor for Block.class

He's talking about, that we have to hardcode an ID for our block and that would conflict with any other mod which uses the same ID. We need to use a registry sync so that isn't a problem.

also adding blocks is gonna require batching into terrain.png

A custom block class could be used to detect if and from where we need to draw from an external png rather than from terrain.png

@sschr15 should be adding a resource loader api, which would probably be something like `FabricLoader.getInstance().getModContainer("mymodid") + "/resources/blah.png"

There’s a lot of features in Block that require dehardcoding
Until there’s resource loader and texture api I’d suggest stalling

Maybe. Speak on discord in #github for quicker responses.

BoogieMonster1O1 · 2020-11-30T01:53:02Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/Pair.java

@@ -0,0 +1,16 @@
+package net.textilemc.textile.utils;
+
+public class Pair<T, M> {


Doesn’t guava have Pair?

If not this should probably go in api base

Guava isn't an included library, so we can decide to keep our own, or use Guava here.

Loader uses guava iirc

Huh? if it does, then could you ready up a commit for using that Pair instead?

BoogieMonster1O1 · 2020-11-30T01:54:21Z

textile-utils-api-v1/src/main/resources/textile-utils-api-v1.accessWidener

@@ -0,0 +1,2 @@
+accessWidener	v1	named
+accessible  method  net/minecraft/block/Block   <init>  (ILnet/minecraft/block/Material;)V


Unnecessary access widener

That AW is necessary for Pair to function.

Really? It wasn't when I used it

Oh yeah, I was using a custom block class

That's because you instantiated your Block (Stone) class, I had to instantiate it as a Block as I don't want to hardcode it to be "Stone" or whatnot.

Why is a Block access widener necessary

Perhaps in the future write a more extensive api that has all modded blocks be instances of a FabricBlock, and thus all with same numerical id, but also have a unique String id, which is checked every time the numerical id is checked, but that would require many mixins to fix that, and that is probably later down the rodd

Look at the top issue. I commented about some ways to do that.

BoogieMonster1O1 · 2020-11-30T01:54:42Z

textile-utils-api-v1/src/main/resources/fabric.mod.json

+  "environment": "*",
+  "accessWidener": "textile-utils-api-v1.accessWidener",
+  "mixins": [
+    "textile-utils-api-v1.mixins.json"


Doesn’t have any mixins

Yeah, need to remove that as said before, leftover from copying module.

BoogieMonster1O1 · 2020-11-30T01:56:10Z

textile-utils-api-v1/build.gradle

+version = '1.0.0'
+
+dependencies {
+	implementation project(':textile-api-base')


Unnecessary dependency

It is unnecessary.

actually keep this but move Identifier to api base

sschr15

Clean up the listed Mixin-related problems and re-request a review, and I'll approve

Thanks to @amusingimpala for doing this.

BoogieMonster1O1

Looks good, just a few extra changes

BoogieMonster1O1 · 2020-12-03T02:54:51Z

textile-utils-api-v1/build.gradle

+}
+
+minecraft {
+	accessWidener "src/main/resources/textile-utils-api-v1.accessWidener"


Why? We need to open up Block.id, Item.id and Block.BLOCKS from being final

doesnt need an access widener

I’m afraid I still don’t understand what you mean. Do you mean 1) that what is being widened is unnecessary, 2) that although it is needed for compilation it is not needed in production, or 3) something else?

it is not necessary to use an access widener here. accessors will suffice

Accessors can modify final variables? I had forgotten that. Okay that makes sense then.

Yep. Adding a @Mutable does the trick

BoogieMonster1O1 · 2020-12-03T02:56:11Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+
+	@Inject(at=@At(value = "INVOKE", target = "Lnet/minecraft/client/MinecraftClient;joinWorld(Lnet/minecraft/world/World;Ljava/lang/String;)V", ordinal = 1), method = "joinWorld(Ljava/lang/String;)V")
+	public void removeConflictOlderVersion(CallbackInfo info) {
+		mapBlockToDat(new File(new File(mcDir, "saves"), name));


use nio
File is gonna block the thread

mcDir.toPath().resolve("saves").resolve(name)

BoogieMonster1O1 · 2020-12-03T02:57:30Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/util/Identifier.java

+		return path;
+	}
+
+	public CompoundTag toTag(CompoundTag tag) {


You don’t need a CompoundTag for this. That just wastes bytes
Store it as a StringTag

What’s the use case of CompoundTag vs Tag?

CompoundTag is a maplike tag
Tag is the base class for tags

BoogieMonster1O1 · 2020-12-03T02:58:35Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+	@Unique
+	private void mapBlockToDat(File saveDir) {
+		if (!(new File(saveDir, "block_registry.dat")).exists()) {
+			System.out.println("Saving Block Registry to .minecraft/saves/" + name + "/block_registry.dat");


use LOGGER.debug here

If LOGGER doesn't exist (which it probably doesn't), just create a @Unique private static final Logger LOGGER. Also please (as the above comment says) use paths instead of File objects...

BoogieMonster1O1 · 2020-12-03T02:59:25Z

...le-utils-api-v1/src/main/java/net/textilemc/textile/utils/RegistrationManagerEntrypoint.java

+
+public class RegistrationManagerEntrypoint implements ModInitializer {
+
+	public static Logger LOGGER = LogManager.getLogger("Textile API");


Suggested change

public static Logger LOGGER = LogManager.getLogger("Textile API");

public static Logger LOGGER = LogManager.getLogger("Textile Utils API");

BoogieMonster1O1 · 2020-12-03T04:48:09Z

imo since this basically only handles the registration of blocks, the module name should be textile-block-api-v1

amusingimpala75 · 2020-12-03T13:55:33Z

I would agree with BoogieMonster, except for Identifier. That should stay in Util

BoogieMonster1O1 · 2020-12-03T13:56:41Z

I would agree with BoogieMonster, except for Identifier. That should stay in Util

Identifiers can be used in any module. That can go in api base.

BoogieMonster1O1 · 2020-12-03T14:03:56Z

honestly anything made as a utility is either

related to a specific thing, or
can be used by multiple things

amusingimpala75 · 2020-12-03T23:00:49Z

I might also consider having the id retriever(s) throw a new ArrayOutOfBounds exceptions citing too many blocks if trying to register more than 256 total.

sschr15

Although Minecraft itself couldn't possibly have used nio libraries, Fabric Loader does, so please use those. Also, Identifier serialization is strange, so I've requested changes there as well.

sschr15 · 2020-12-13T23:43:17Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+	@Unique
+	private void mapBlockToDat(File saveDir) {
+		if (!(new File(saveDir, "block_registry.dat")).exists()) {
+			System.out.println("Saving Block Registry to .minecraft/saves/" + name + "/block_registry.dat");


If LOGGER doesn't exist (which it probably doesn't), just create a @Unique private static final Logger LOGGER. Also please (as the above comment says) use paths instead of File objects...

sschr15 · 2020-12-13T23:47:25Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+	}
+
+	@Unique
+	private void mapBlockToDat(File saveDir) {


Path saveDir

sschr15 · 2020-12-13T23:47:52Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+	}
+	@Inject(at=@At(value = "INVOKE", target = "Lnet/minecraft/client/MinecraftClient;joinWorld(Lnet/minecraft/world/World;Ljava/lang/String;)V", ordinal = 2), method = "joinWorld(Ljava/lang/String;)V")
+	public void removeConflictOlderVersion2(CallbackInfo info) {
+		mapBlockToDat(new File(new File(mcDir, "saves"), name));


mcDir.toPath().resolve("saves").resolve(name)

sschr15 · 2020-12-13T23:48:09Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+
+	@Inject(at=@At(value = "INVOKE", target = "Lnet/minecraft/client/MinecraftClient;joinWorld(Lnet/minecraft/world/World;Ljava/lang/String;)V", ordinal = 1), method = "joinWorld(Ljava/lang/String;)V")
+	public void removeConflictOlderVersion(CallbackInfo info) {
+		mapBlockToDat(new File(new File(mcDir, "saves"), name));


mcDir.toPath().resolve("saves").resolve(name)

sschr15 · 2020-12-13T23:49:17Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+
+	@Unique
+	private void mapBlockToDat(File saveDir) {
+		if (!(new File(saveDir, "block_registry.dat")).exists()) {


!Files.exists(saveDir.resolve("block_registry.dat"))

sschr15 · 2020-12-13T23:50:02Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+				}
+			}
+			try {
+				Util.writeCompressed(tag, new FileOutputStream(new File(saveDir, "block_registry.dat")));


Files.newOutputStream(saveDir.resolve("block_registry.dat")

sschr15 · 2020-12-13T23:50:06Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+			System.out.println("Matching old Block Registry read from .minecraft/saves/"+name+"/block_registry.dat");
+			CompoundTag tagFromFile;
+			try {
+				tagFromFile = Util.readCompressed(new FileInputStream(new File(saveDir, "block_registry.dat")));


Files.newInputStream(saveDir.resolve("block_registry.dat")

sschr15 · 2020-12-13T23:52:41Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/mixin/BlockCacherMixin.java

+				}
+			}
+		}
+		throw new NullPointerException("Could not find block! You should only see this if you removed the mod which it was in, in which case your world will have corrupted!! Please make a new World!!");


Perhaps don't throw an NPE, instead send a message to the player on world load to tell the player that a block may be missing?

sschr15 · 2020-12-13T23:54:49Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/util/Identifier.java

+	public CompoundTag toTag(CompoundTag tag) {
+		CompoundTag subtag = new CompoundTag();
+		subtag.putString("namespace", this.getNamespace());
+		subtag.putString("path", this.getPath());
+		tag.put("nameId", subtag);
+		return tag;


Suggested change

public CompoundTag toTag(CompoundTag tag) {

CompoundTag subtag = new CompoundTag();

subtag.putString("namespace", this.getNamespace());

subtag.putString("path", this.getPath());

tag.put("nameId", subtag);

return tag;

public CompoundTag toTag(CompoundTag tag) {

tag.putString("nameId", this.getNamespace() + ':' + this.getPath());

return tag;

sschr15 · 2020-12-13T23:57:06Z

textile-utils-api-v1/src/main/java/net/textilemc/textile/utils/util/Identifier.java

+	public static Identifier fromTag(CompoundTag tag) {
+		return new Identifier(tag.getString("namespace"), tag.getString("path"));
+	}


According to the toTag method just above, you save this to a nameId entry. You should then retrieve the identifier the same way.

Suggested change

public static Identifier fromTag(CompoundTag tag) {

return new Identifier(tag.getString("namespace"), tag.getString("path"));

}

public static Identifier fromTag(CompoundTag tag) {

String[] value = tag.getString("nameId").split(":");

return new Identifier(value[0], value[1]);

}

serialization should not be done directly to a compound tag. nameId sounds weird.

in fact, the compound tags in fromTag and toTag are meant to be of that object, so the only data you read is the data you’ve serialized

and since Identifiers don’t use compound tags anymore, this should just be a toString and a tryParse

(so that the caller can choose any key name and so as to not violate the method contract)

Add textile-utils-api-v1

21aac07

Bikeshed:tm: on new branch

PhoenixVX requested a review from BoogieMonster1O1 November 30, 2020 01:43

BoogieMonster1O1 requested changes Nov 30, 2020

View reviewed changes

Just to fix the issues about leftover module stuff.

c002273

sschr15 mentioned this pull request Nov 30, 2020

Block Conflict Resolution Possibility #4

Open

sschr15 reviewed Nov 30, 2020

View reviewed changes

Add BCR utilities to textile-api

18cc781

Thanks to @amusingimpala for doing this.

BoogieMonster1O1 requested changes Dec 3, 2020

View reviewed changes

sschr15 requested changes Dec 13, 2020

View reviewed changes

		@@ -0,0 +1,16 @@
		package net.textilemc.textile.utils;

		public class Pair<T, M> {

		@@ -0,0 +1,2 @@
		accessWidener v1 named
		accessible method net/minecraft/block/Block <init> (ILnet/minecraft/block/Material;)V


		public class RegistrationManagerEntrypoint implements ModInitializer {

		public static Logger LOGGER = LogManager.getLogger("Textile API");

Add textile-utils-api-v1 #3

Are you sure you want to change the base?

Add textile-utils-api-v1 #3

Uh oh!

Conversation

PhoenixVX commented Nov 30, 2020

Uh oh!

BoogieMonster1O1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amusingimpala75 Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhoenixVX Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amusingimpala75 Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sschr15 left a comment

Choose a reason for hiding this comment

Uh oh!

BoogieMonster1O1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amusingimpala75 Nov 30, 2020 •

edited

Loading

PhoenixVX Nov 30, 2020 •

edited

Loading

amusingimpala75 Nov 30, 2020 •

edited

Loading

amusingimpala75 Dec 3, 2020 •

edited

Loading