SHsuperCM / CITResewn

Fabric implementation of mcpatcher's cit
MIT License
154 stars 76 forks source link

Contents of CIT directory seem not to be added to the texture atlas #275

Closed Traister101 closed 1 year ago

Traister101 commented 1 year ago

Right off the bat this is not an issue where CIT textures aren't working but speculars and normals, textures used for storing things like reflectiveness in shaders which are not working. To justify why I think this is a Resewn issue and not on Iris/shader packs is that in Optifine speculars and normals for CIT textures work as expected. That is to say that like with a vanilla texture you simply place it in the same directory. I highly suspect this is an issue relating to the texture atlas or how the CIT textures are registered so it might require a significant rework to the code to fix.

Attached is a basic resource pack to better demonstrate this issue. As well as my configuration file for Complementary Reimaged v2.2 which is a shader with labPBR specular support (not relevant to issue just the format of my speculars) and the one I personally use. I also tested Complementary v4 and BSL which have the same behavior. CIT specular debug pack.zip ComplementaryReimagined_r2.2.zip.txt

Each variant uses identical specular textures to the ones used when no CIT is applied IE the vanilla texture is being used. The CITs are for 3 items including the elytra. They are the Bucket, the Totem of Undying and the Elytra because this issue also effects the "armor" texture when worn.

These three items have 3 "variants" accessed via renaming. They are "resewn" for simply placing the specular texture in the same location the base texture is in this pack that's with the properties file, "vanilla" placing both the base and specular texture under a vanilla texture path which is added to the atlas for this pack that's "textures/item/cit" and "manual" placing both the base and specular texture under a newly created directory "textures/cit" which is manually added to the atlas in blocks.json.

These three variants are easily distinguishable in game through color coding, resewn has blue textures, vanilla are magenta and manual are green. There is no particular reason for these colors it just makes it easy to verify the correct texture is being used.

Of these variants resewn are the only ones which exhibit no specular effects, when using Optifine however it has specular effects for all 3.

The following screenshots are rather self explanatory but I'll give a quick explanation of what is expected. Firstly each screenshot has the held items name to easily tell them apart aside from the solid color differences. The totem on the left (offhand) is the vanilla texture, it is somewhat reflective and has glowing eyes. The resewn totem should reflect the vanilla totem as well as have glowing eyes (better seen in the other two working variants). 2023-06-26_20 00 20

This is the CIT totem with it's assets under a vanilla path IE the "textures/item/cit" one. Like the previous it should have glowing eyes and be somewhat reflective. You can clearly see what would be glowing eyes if it weren't for the solid colors as well as a slight reflection of the vanilla totem in the offhand. 2023-06-26_20 00 32

This is the CIT totem with it's assets under the manually created "textures/cit" path which is added to the atlas manually via blocks.json. This like the previous totem should be reflective and have glowing eyes. In this totem you can clearly see the reflection of the vanilla totem in the offhand due to it's lighter color. 2023-06-26_20 00 44

Here is a screenshot of the resewn variant working under Optifine with no other changes. 2023-06-26_20 20 33

Fixing this issue might be as simple as adding the cit directory to the atlas which doesn't seem possible through a resource pack. At the very least I was unable to do so by using the relative "../optifine/cit" path.

SHsuperCM commented 1 year ago

So this is less of an issue with the atlases and more of an issue with how CIT Resewn presents texture identifiers to the game. Most likely a duplicate of #149 and just that special handling was added in continuity for emissives but there isnt one in Iris for speculars and normals(which is totally understandable).

Right now CIT Resewn just tells the game a unique absolute path which it handles internally but this is not expected behavior that these mods take into account.

The model rewrite already has a plan to solve this issue by internally moving old "legacy" textures to modern identifiers so they'd get support from other mods' special texture features.

For now I am marking this as a duplicate of #149 as it should be resolved once a proper "correct" fix is added by CIT Resewn which would fix both issues.

Traister101 commented 1 year ago

Strange that there's a difference when referencing textures in the atlas then? From the sounds of it it should have the same behavior as the (implicit) absolute path when pointing to a local CIT texture. I guess since the absolute path is also a modern identifier it just works? Appreciate the clarification.

Glad this will be getting fixed eventually as otherwise I'd be looking at least an hour of tedious work moving textures and editing CIT properties. Thanks for your work!

SHsuperCM commented 1 year ago

If you want the details on what really happens I suggest reading #149. Essentially absolute identifiers are not "modern", and I have to have specual handling for them(currently differentiated by suffixing them with their file extensions .png). The mods that rely on the file extension not being there will attempt to use ".png_e.png" which ofc fails. I suppose the same is happening with other suffix based asset checks

Traister101 commented 1 year ago

So basically my question is why does an absolute path sometimes work and sometimes not? Even though I'm pointing to an texture that should have a modern identifier (my manual or "automatic" atlas stuff) it should still be using an absolute identifier for the actual CIT right? Or in other words we point to a texture and include the extention but someplace else (the identifier vanilla creates) does it "correctly" (no extension). IE how does the presence of a modern identifier make the absolute identifier correctly exhibit the specular effects even though it seems like it shouldn't?

My comment of "I guess since the absolute path is also a modern identifier so it just works?" is cause I can only guess things somehow just work™ cause a non extension identifier exists and somehow it's recognized that the two separate identifiers are for the same asset so Iris manages to load the specular map. Far as I understand none of my examples should have worked at all yet two of them did, they both through being added to the texture atlas had modern identifiers created for them automatically I'd assume. Even though we don't use these identifiers the specular effects show up which I guess could be a really lazy and crude way to fix the issue?

SHsuperCM commented 1 year ago

My guess would be that it just depends on how I set up the requirement for absolute paths but honestly, I just dont know. Afaik it just shouldnt work, and I am specifically not interested in looking into why it works because it's going to be rewritten regardless..

I'm also still having a little trouble understanding what you mean when you say "modern" identifiers. Identifiers in the texture space are only supposed to specify the texture's name(without the .png) and in newer minecraft it locates where they are in the assets by the texture atlas specifications in the resourcepack. CITR just injects somewhere inside the game and checks if the identifier already has a .png extension and then it treats it as an absolute path in the assets, which is not a thing that other mods expect to happen. That's all really..

I dont really know why it would work beyond that but if you want to look into it, you're free to do so, this is exactly the reason I love open source, anyone can look at the code if they want to learn something.

Traister101 commented 1 year ago

I thought you had used the term to describe identifiers without the extension but it seems I misread

The model rewrite already has a plan to solve this issue by internally moving old "legacy" textures to modern identifiers so they'd get support from other mods' special texture features.

As for looking into it I'd like to, honestly I started out by trying to find some sort of bug in the code cause I was convinced the textures were being registered under a incorrect path or something of the sort. I mostly have a bearing on what's going on thanks to modding in 1.12 Forge but well stuff is different enough where I don't have a firm grasp on it well as the mod being Fabric, not Forge. I am rather interested in being a complete unga bunga cave man and crudely making identifiers for stuff without touching however the mod currently works just to see what happens but honestly I don't wanna deal with whatever setup is involved for a dev instance. I hear it's a lot less of a hassle with Fabric but bleeh.

Got a lot to do just with the resource pack I've been messing with don't really want to spend a day or so figuring out fabric enough to maybe come up with a fix to a problem that at the end of the day I only really notice cause I'm a perfectionist. Speaking of I did try renaming a few CIT speculars in the style of .png_s.png which did not seem to work. If it had I'd happily deal with that stupidity till resewn eventually released a fix lol.

If I do end up messing with it I guess I'll say what the results were here? The whole learning Fabric + 1.19 MC is just not that appealing though...