MehVahdJukaar / Supplementaries

Other
125 stars 91 forks source link

log flooded with sign texture errors #934

Closed Linguardium closed 4 months ago

Linguardium commented 5 months ago

https://github.com/MehVahdJukaar/Supplementaries/blob/73fcd9508fb2f88aa0609bfe049e380fe5330f5f/common/src/main/java/net/mehvahdjukaar/supplementaries/dynamicpack/ClientDynamicResourcesGenerator.java#L236-L265

This should be an opt-in situation not a global situation. You are throwing errors for things that are not opting into your system and then logging those errors blaming other mods.

error snippet

MehVahdJukaar commented 5 months ago

It is opt in. You can disable enhanced hanging signs (and hence that stuff) via configs

MehVahdJukaar commented 5 months ago

Also that's not my system at all. That's vanilla system. If you add a wood type and it's texture isn't even in the vanilla maps then it's not done properly

Linguardium commented 5 months ago

your mods have errors being thrown for a global, user-opt-out system, based on assumptions you make about what might exist in the vanilla game without actually checking. you throw errors based on your unchecked assumptions mentioning a non-existent mod resource. Thats entirely a "your system" issue not a vanilla one

MehVahdJukaar commented 5 months ago

I'd that's what's failing then that's a bug. It's only supposed to log when those textures don't exist not when that returns null

Linguardium commented 5 months ago

Vanilla adds all wood types to the RenderLayer map, even if they arent used for hanging signs. the renderer would be using renderer.getTextureId which would handle the texture id for that renderer.

MehVahdJukaar commented 5 months ago

Alright so 2 things: I've tried so hard to import any of those mods (blockus, minecelss, ad astra and spectrum) but failed. They all have many dependencies which either are unliested like its the case for spectrum that has 3, one of them cant be even found on curseforge, or use other dependencies with JIJ which my architectury setup cant import. In the case of better archeology yungs api throws some error. In short I cannot import and test any of them.

Secondly i have checked the toVanilla call and that should just get the hanging sign block (which we know exist otherwise it wouldnt have appeared in that for loop to begin with, and then just call its own .type() method which returns the vanilla wood type so that should never be null

Linguardium commented 5 months ago

yes, importing is a no-go for many of them. ideally you should be applying only to vanilla and to mods that explicitly opt-in to your system.

without logging the actual error, its hard to say whats actually failing but i expect it either isn't finding a hanging sign for them (thus returning null) or you are trying to load the default texture id path instead of using the block's own renderer TextureId.

I would suggesting doing some checks at the head of the loop to verify the mod has both opted into your system and that the expected path exists. If either isn't true, dont process that wood type.

MehVahdJukaar commented 5 months ago

from what i can see (out of dev) all of those are failing because they are not adding stuff to the vanilla map. Now yes the rendererd returns that texture too and is arguably more accurate. I've just updated the code to fallback to that. Still one should still register stuff properly to the vanilla maps. That functon has a name and is not null by default so by contract it should not return null for any wood type. System hence remain opt out as if it would faul would necessairly mean that a mod isnt using the "vanilla system". Also what would be the need to register a custom tile renderer for hanging sign whent the vanilla one works the same?

Also noticed the code was missing a check for the existance of signs to behin with. Stil believe that to be a somehwat of error from mods that add wood but not hanging sign but that wast intented so that was resolved (gets rid of that one "alpha" wood type error)

Linguardium commented 5 months ago

if i recall correctly (also not in dev) BlockFamily doesnt list hanging signs, but does list signs. Vanilla is not consistent on what does and doesnt exist. Helper methods that generate a list of potential texture paths should not be expected to be a source of truth.

The vanilla renderer is fairly hardcoded. Sometimes its better to have your own custom renderer to be more flexible, hanging sign or not.

Not adding the hanging sign might just be an oversight from updating mods from old versions. if you see it, you may want to open issues on their trackers to let them know its missing,