fabric: Resource loader breaks vanilla behaviour

Hello. By default, DefaultResourcePack#findInputStream returns null if resource was not found, however this mixin introduces a different behaviour that causes exception to be thrown. Could a PR be opened to implement a fix for that? (I can do that by myself if necessary) Thanks

See https://github.com/xxDark/BetterLoading/issues/2#issuecomment-964729312

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Comments: 21 (10 by maintainers)

Most upvoted comments

I haven’t been able to check the code yet, but catching the exception just to have another one be thrown in calling code seems quite wasteful. I think it’s fair to assume that this “behavior change” is not observable to normal outside code, so I think it would be better if you caught the exception yourself. I’m open to discussion though.

(On a side note, I’m curious to see if this particular optimization in your mod changes how long it takes to load a real-world modpack like AOF4 or not. I did similar experiments a while ago and these kinds of changes didn’t seem to help as far as performance was concerned.)

I will be able (to hopefully) confirm that it in fact does speed up performance if this issue gets either resolved in one way or another. I don’t want to catch all IOException’s due to overhead of backtrace capturing.

The fabric mixin looks wrong to me in 2 ways.

This broken behaviour seems to have been introduced by this requested refactoring https://github.com/FabricMC/fabric/pull/1564#discussion_r669205725

Originally the PR overwrote

   public InputStream open(ResourceType type, Identifier id) throws IOException

which does throw an IOException for not found.

But the refactoring made it overwrite

   protected InputStream findInputStream(ResourceType type, Identifier id)

Which returns null for not found and doesn’t even throw an IOException.

The overwritten method has changed the signature to

       @Nullable
	@Overwrite
	public InputStream findInputStream(ResourceType type, Identifier id) throws IOException {
		return fabric_mcJarPack.open(type, id);
	}

which makes the method public and adds a throws IOException.

The mixin delegates to a Jar(Directory)ResourcePack backed by the minecraft.jar which will throw a ResourceNotFoundException if the resource is missing.

A similar mistake was made for at least getInputStream.

Any fix on our side would be to add a try/catch block inside our findInputStream, so unfortunately I have nothing better to suggest. Throwing an exception should still be faster than checking for existence and then opening the resource given that most resources in the minecraft namespace are provided by the default pack.

Alright, I think I know how I can resolve this for now without us arguing about who does not want to catch IO exceptions!