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 xxDark/BetterLoading#2 (comment)

The fabric mixin looks wrong to me in 2 ways.

This broken behaviour seems to have been introduced by this requested refactoring
#1564 (comment)

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.

The second way this looks wrong is that the vanilla default resource pack has a static field called "resourcePath" which is used in preference to the minecraft.jar when it is not null.

I don't know where this is set (maybe just by MC developers during testing?).
But the Fabric Mixin does not reproduce this behaviour.

The second way this looks wrong is that the vanilla default resource pack has a static field called "resourcePath" which is used in preference to the minecraft.jar when it is not null.

I don't know where this is set (maybe just by MC developers during testing?). But the Fabric Mixin does not reproduce this behaviour.

Presumably when mojang develops the game, that's why I omitted it from the mixin.

I'll have to check the code again, there might have been a good reason or it might have been a mistake on my end.

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

I don't want to catch all IOException's due to overhead of backtrace capturing.

Same thing for me! I don't want to catch then throw the exception again. 😄 You catching an IOException thrown by findInputStream should be equivalent to fabric doing it, but only when your mod is loaded. I think?

Well, fabric might do something like:

if (delegatingPack.contains(...))
    return delegatingPack.open(...);

P.S: Maybe my mod will be merged directly to Fabric, if that's the case then, I don't mind doing that, becaue I see how performance & load time degraded since last time I checked.

I don't want to catch all IOException's due to overhead of backtrace capturing.

Same thing for me! I don't want to catch then throw the exception again. 😄
You catching an IOException thrown by findInputStream should be equivalent to fabric doing it, but only when your mod is loaded. I think?

That looks slower in the very common case where the resource exists though.

I updated my previous comment
But either way, currently vanilla calls contains method every single time, it is already slowed down.

That's not a reason to slow it down even more though. :P

That's not a reason to slow it down even more though. :P

Right, but as a temporary solution, is there anything I can do to resolve this on my end without catching all exceptions?

Can't you catch IOException? Not sure what you mean by "all exceptions".

Well yeah, I meant IOException, but I still don't want to do that either...

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

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.

Can't you catch IOException?

This doesn't sound like a good solution.
Anybody that wants to do something with the default resource pack is going to have to do something like:

// Reproduce vanilla behaviour in presence of fabric
InputStream is;
try {
    is = defaultResourcePack.findInputStream(x, y);
} catch (Exception e) { // can't catch IOException because it's not declared by vanilla
    is = null;
    // this would also "eat" RuntimeExceptions hiding possibly more important issues unless the user does
    if (e instanceof RuntimeException runtime) throw runtime;
}

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.

If the most common case is resource found, catching the exception and returning null won't be an issue.
It will be a noop for this common case.

If it is a performance overhead (because there are lots of misses), a solution would be to mixin a more performant ResourcePack.openFile() to avoid the ResourceNotFoundException being created in the first place.

e.g. for ZipResourcePack

   public InputStream fabricOpenFile(String name)  {
      try {
          ZipFile zipFile = this.getZipFile();
          ZipEntry zipEntry = zipFile.getEntry(name);
          if (zipEntry == null) {
-             throw new ResourceNotFoundException(this.base, name);
+            return null;
          } else {
             return zipFile.getInputStream(zipEntry);
          }
        }
        catch (IOException e) {
              // can still happen if there is something wrong with the zip file
             // e.g. failure of a network mount, corrupted file, etc.
             return null;
        }
   }

Solved the problem by grabbing resource pack instance that Fabric injects and added some checks for it, however, panorama appears completely gray (without Fabric's API, everything works fine). Gonna figure out if that's my mistake, or there are another underwater rocks.

Ah yes, the grey panorama. It's usually an indicator of a problem with the MC resource pack (for example, we got this in Architectury when the minecraft jar patching messed up the resources somehow, although on Forge), but not sure where the problem comes from exactly.

Ah yes, the grey panorama. It's usually an indicator of a problem with the MC resource pack (for example, we got this in Architectury when the minecraft jar patching messed up the resources somehow, although on Forge), but not sure where the problem comes from exactly.

Nah, it was my fault. Because Fabric overwrites findInputStream, the behaivour for ClientDefaultResourcePack was also changed, so I had to adjust some code.

Good that you managed to fix it.

@warjort The behavior change is not visible to other normal mods because they 1) have no reason to be using the default pack directly, and 2) findInputStream is protected. Catching the exception and then re-throwing it later might be slower, or it might not be. But in any case I would rather not spend time trying to fix a performance problem that this could introduce.