CITResewn icon indicating copy to clipboard operation
CITResewn copied to clipboard

fixed: any unknown condition breaks cit completely

Open HiWord9 opened this issue 1 year ago • 5 comments

So all i did is i changed ConstantCondition.FALSE to TRUE that will be attached to list of conditions for cit in case a condition is unknown for CITR.

I was making a new feature for my mod, this feature requares a parameter in .properties file. So i implemented it, started testing and my feature works well, but the cit itself breaked. I started looking for cause of its issue and fould out that code adds unreachable condition. I tested on OptiFine and there the cit works fine with my feature. I thought of possibility to use api to register it as my mod's condition, but api is deprecated and i do not want for resource pakcs work only with my mod. I also thought about using commented line in .properties file but its tooooooo strange and not practical and difficult to implement as commented lines are ignored by default and its just not cool as the .properties funcional already gives ability to get values from it.

So, why to make cit unreachable? If there is a misstake in properties file it will be ignored competely. If other mod uses CITR's api (what is most likely not) to add its custom condition, cits with this condition won't work at all without this mod. And as packs are not depend on mod loader, on forge (with OF) it will work, but not with cit resewn. I mean this pull request was not made esspecially for me and my goals, i made it cause i found an issue while reaching my goals and fixed it.

This change should not break any already existed resource pack.

OR may be to add some tag to the beggining of condition line as "IGNORE" or idk mb "::" or any else that will tell CITR that this condition should be ignored and not parsed through?

HiWord9 avatar Feb 02 '24 23:02 HiWord9

So the default of an unknown condition definitely should stay yielding false imo, so it's not really a bug to be fixed and I'd say it's working as intended. However there probably should be a way for other mods to add these flags without breaking the resourcepack. Note however that this is not a condition really but more of a type's effect declaration.

Perhaps a short flag is in order to mark certain lines as "ignore by default"..

SHsuperCM avatar Mar 27 '24 07:03 SHsuperCM

So, what i just made is i added a condition to skip a line in properties file if it starts with "%", it means that other mods are able to read their custom values from prop file when citr know that this condition should not be assigned to cit. It also means that if value is something unknown and not marked as citr ignored the citr still will set it as False condition.

HiWord9 avatar Mar 27 '24 19:03 HiWord9

I think this might be the best option but I am not sure about the syntax and where it's been implemented. I'll think about it.

SHsuperCM avatar Mar 29 '24 07:03 SHsuperCM

So, i see lately you've been porting citr to 1.21, and it mean somewhen in nearest future citr will get an update. Because of this, i would like to restore the conversation about this pr. Lastly, u said that the way i've implemented it is not good enough. So, what actually the right way i should implement it? Mb not exactly in properties file reader but in some logic where readed properties are parsed? I may change the pr if its needed.

HiWord9 avatar Sep 03 '24 16:09 HiWord9

Still have to think about it, in the time since I've heard some good points for the case of having unknown conditions be treated as passing. But top priorities right now are 1.21 and redoing fabric api integration, this will have to be looked at after that is done.

SHsuperCM avatar Sep 04 '24 04:09 SHsuperCM