PorktoberRevolution / ReStocked

Kerbal Space Program Art Revamp
98 stars 44 forks source link

Fix cfg syntax errors #988

Closed HebaruSan closed 2 years ago

HebaruSan commented 2 years ago

Hi, @ChrisAdderley!

I'm using ReStock to test some cfg parser code for KSP-CKAN/CKAN#3525, and I found a few minor syntax errors; descriptions/explanations below. This pull request fixes them.

ru.cfg

restock-lights.cfg

restock-engines-liquid-25.cfg

restock-engines-liquid-375.cfg

restock-structural-truss.cfg

RS_HTCH_625.cfg, RS_LGHT_Airlock_Green.cfg, RS_LGHT_Airlock_Red.cfg

This is where I anticipate some possible controversy.

RS_LGHT_Box_1.cfg

Poodmund commented 2 years ago

This is really helpful, thank you, HebaruSan.

The #node_stack_bottom02 on Line 23 of "Distribution/Restock/GameData/ReStock/Patches/Engine/restock-engines-liquid-375.cfg" can just be removed from the config. The node values are identical to the bottom node present in the stock config.

With regards to the proxy lines being move inside the config nodes, I defer that to @drewcassidy for confirmation of acceptability. Has this been tested in-game, @HebaruSan? I know you have said that this separate parser does allow for it to follow the correct config node syntax location but just checking for thoroughness.

Other than that, all looks good to me. :D Was this CKAN cfg parser run against the whole ReStocked repo?

HebaruSan commented 2 years ago

The #node_stack_bottom02 on Line 23 of "Distribution/Restock/GameData/ReStock/Patches/Engine/restock-engines-liquid-375.cfg" can just be removed from the config. The node values are identical to the bottom node present in the stock config.

Good to know, done. :+1:

Has this been tested in-game, @HebaruSan? I know you have said that this separate parser does allow for it to follow the correct config node syntax location but just checking for thoroughness.

That's a good point: No, I have not tested this in-game. Someone associated with the ReStock team should definitely check it out to be safe (I don't know what proxy does as I have not worked on props, so I would not know what to look for). My investigation was based purely on how the stock parser appears to behave at a code level.

Was this CKAN cfg parser run against the whole ReStocked repo?

I installed ReStock manually and then ran the parser against everything in my GameData (along with JNSQ, see Galileo88/JNSQ#44; I'll probably be trying it with RP-1 soon :crossed_fingers:). So it should cover everything in the download, but if there are any files in the repo that are not distributed in the downloads, those would not have been scanned.

Poodmund commented 2 years ago

That's a good point: No, I have not tested this in-game. Someone associated with the ReStock team should definitely check it out to be safe (I don't know what proxy does as I have not worked on props, so I would not know what to look for). My investigation was based purely on how the stock parser appears to behave at a code level.

It seems as though this specific parser loads all matching proxy lines into a collection of some kind, probably a list. It looks able to handle multiple proxy lines but it also looks like it uses the first 'name' entry and ignores the rest so we can only have one prop per file. This shouldn't be an issue for the config files in question here.

With reference as to what these 'proxy' lines actually do, if I test this, in-game, with the lines within the config node and outside the node, what behavioural changes should I be looking for it this breaks, @drewcassidy? What do these lines do?

drewcassidy commented 2 years ago

Good question. I don't know what the proxy does

HebaruSan commented 2 years ago

Exciting! I'll resume my quest for documentation of prop configs...

... well, no documentation yet, but I did manage to discover that B9 has had a prop in this style for 8+ years:

https://github.com/blowfishpro/B9-Aerospace/blob/09766a1ac43bd25d4391c62da42fbc6d4c92e690/GameData/B9_Aerospace/Props/B9_MFD/B9_MFD.cfg#L1379

NearFutureProps is a mix; it has 215 props with the proxy outside the node as stock does, and 24 with the proxy line inside the node as this pull request is doing.

ASET's props are all stock-style (proxy line outside node). (Last visited the forum Sept 2019, so probably won't respond to a question by PM.)

HebaruSan commented 2 years ago

Good question. I don't know what the proxy does

I found one discussion of it on Discord between you and @taniwha in May 2020:

image

So I guess it isn't for the game, but rather helps with editing IVAs in Unity?

drewcassidy commented 2 years ago

Sounds about right. I don't know if PartTools needs them to be there or not, but If it doesn't I can just put them back temporarily. Of course if it doesn't hurt anything in game I'd rather just keep them

drewcassidy commented 2 years ago

Nevermind, Part Tools literally never reads that line

HebaruSan commented 2 years ago

Is PartTools the same thing as PropTools? I am not familiar with these things.

drewcassidy commented 2 years ago

PartTools is the unity plugin for making parts and IVAs. I don't know what PropTools is

HebaruSan commented 2 years ago

OK, maybe @taniwha was referring to something else, then.

Poodmund commented 2 years ago

It is part of PartTools it seems: https://www.kerbalspaceprogram.com/api/class_prop_tools.html

And also the PropTools threads on the forum date back to like 2014.