Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cfg syntax errors #988

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Conversation

HebaruSan
Copy link
Contributor

@HebaruSan HebaruSan commented Mar 25, 2022

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

  • Extra spaces inside of localization keys after the #, probably results in the Russian translation not working in-game

restock-lights.cfg

  • Missing = after displayName, probably results in those variants not having working display names
  • Duplicate pitchMin properties, probably no impact

restock-engines-liquid-25.cfg

  • Extra s character after close brace (maybe an attempt to press Ctrl+s without enough force on Ctrl?), probably no impact

restock-engines-liquid-375.cfg

  • = on two lines trying to delete MODEL nodes, which means MM will instead try to delete a key with name MODEL instead of a node, impact uncertain depending on how bad it is to have two MODEL nodes in a PART
  • A key starting with # that looks like the intent was to comment it out, now changed to //

restock-structural-truss.cfg

  • Duplicated GAMEOBJECTS node headers without the node contents, probably no impact

RS_HTCH_625.cfg, RS_LGHT_Airlock_Green.cfg, RS_LGHT_Airlock_Red.cfg

This is where I anticipate some possible controversy.

  • proxy key set outside of any config node, now moved into the PROP
    I checked stock, and this is present there; it appears the loader for props doesn't use the proper ConfigNode parser but instead basically greps the lines of the file looking for the values it cares about. However, this ad hoc mini-parser does appear to also allow "correct" syntax, so this should be OK to fix. (I would submit a fix pull request for stock if I had that option. 😛)

RS_LGHT_Box_1.cfg

  • Same proxy fix as above
  • An incompletely commented out MODULE node results in a bare {...} sequence, now fully commented out

@drewcassidy drewcassidy added the bug bug pertains to non-art issues label Mar 26, 2022
@Poodmund
Copy link
Collaborator

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 HebaruSan force-pushed the fix/cfg-syntax branch 2 times, most recently from 762940b to ad75736 Compare March 26, 2022 18:07
@HebaruSan HebaruSan changed the title Fix syntax errors Fix cfg syntax errors Mar 26, 2022
@HebaruSan
Copy link
Contributor Author

HebaruSan commented Mar 26, 2022

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

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 🤞). 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
Copy link
Collaborator

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
Copy link
Member

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

@HebaruSan
Copy link
Contributor Author

HebaruSan commented Mar 26, 2022

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

Nevermind, Part Tools literally never reads that line

@HebaruSan
Copy link
Contributor Author

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

@drewcassidy
Copy link
Member

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

@HebaruSan
Copy link
Contributor Author

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

@Poodmund
Copy link
Collaborator

Poodmund commented Mar 28, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bug pertains to non-art issues
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants