Config Rewrite#183
Conversation
Finished DriveableType, but not yet tested.
Next step, testing.
…ved behaviour and validation.
…dling of passengers + wheels. Discovered ShootableType needs a rework.
# Conflicts: # src/main/java/com/flansmod/client/model/RenderGun.java # src/main/java/com/flansmod/common/FlansMod.java # src/main/java/com/flansmod/common/guns/BulletType.java # src/main/java/com/flansmod/common/guns/GunType.java # src/main/java/com/flansmod/common/guns/ShootableType.java # src/main/java/com/flansmod/common/paintjob/PaintableType.java
Unknown025
left a comment
Unknown025
left a comment
There was a problem hiding this comment.
Looks good overall, nice to see a modernization of config handling. Only major concerns would be case sensitivity (which seemed to be hit or miss in the old implementation, but crucial for old packs to work the same way) and referencing model classes in server side contexts.
| if (FlansMod.printStackTrace) { | ||
| ex.printStackTrace(); | ||
| } | ||
| FlansMod.logPackError(file.name, packName, shortName, "Erorred while reading Driver/Pilot", null, ex); |
There was a problem hiding this comment.
Should probably say "Errored" instead.
There was a problem hiding this comment.
Yup that's correct :)
| onRadar = ConfigUtils.configBool(config, "OnRadar", onRadar); | ||
|
|
||
|
|
||
| splits = ConfigUtils.getSplitsFromKey(config, new String[] { "AddParticle", "AddEmitter" }); |
There was a problem hiding this comment.
Is this still case insensitive? The original code used equalsIgnoreCase.
There was a problem hiding this comment.
| protected void read(ConfigMap config, TypeFile file) { | ||
| super.read(config, file); | ||
| try { | ||
| model = FlansMod.proxy.loadModel(modelString, shortName, ModelAAGun.class); |
There was a problem hiding this comment.
This might fail on the server side. Even though the server side proxy won't crash, referencing ModelAAGun.class might be enough on its own.
There was a problem hiding this comment.
That's a good point, it was previously if'ed out with Side.Client before I think. I'll test it out.
| String typeString = ConfigUtils.configString(config, "AttachmentType", type.toString()); | ||
| type = EnumAttachmentType.get(typeString); | ||
|
|
||
| model = FlansMod.proxy.loadModel(modelString, shortName, ModelAttachment.class); |
There was a problem hiding this comment.
Same here, referencing the model class server-side might cause an exception and not load the remainder of the config file.
| deployableModelString = ConfigUtils.configString(configMap, "DeployedModel", null); | ||
| deployableModel = FlansMod.proxy.loadModel(deployableModelString, shortName, ModelMG.class); | ||
|
|
||
| model = FlansMod.proxy.loadModel(modelString, shortName, ModelGun.class); |
Iirc this is now equivelant to equalsIgnoreCase for config keywords which shouldn't cause any issues afaik |
|
I think I've resolved all issues there, I also tested on a local server which I forgot to do before - could you give it a quick check @Unknown025 please? |
Unknown025
left a comment
Unknown025
left a comment
There was a problem hiding this comment.
Project does not compile due to unresolved references.
| import net.minecraft.item.ItemStack; | ||
| import net.minecraft.nbt.NBTTagCompound; | ||
| import net.minecraft.nbt.NBTTagList; | ||
| import org.classpath.icedtea.Config; |
There was a problem hiding this comment.
What is this for? I'm unable to compile the project due to this reference here. It's present in GunType, ShootableType, PartType, and ToolType.
There was a problem hiding this comment.
Ahhh... Rip I missed that sorry. I think that's something IntelliJ inserts when you type in Config instead of ConfigMap. That can definitely be removed.
3 participants
Description of changes
Completely replace the config loading system with a reworked system. Instead of reading each line of a file with a long list of if/else statements, all lines are instead read into a ConfigMap, then each type file uses methods from ConfigUtils to parse int/float/bool, read a string or retrieve splits.
Each config read using logic that can throw exceptions should be individually wrapped by a try/catch, there are only a few errors which should cause parsing to fail. Furthermore, a standardized logging system has been introduced with FlansMod.logPackError which can print filename, packname, shortname, a message, the split that caused an error and the exception. This should help pack makers easily see issues in their packs.
There's also a small amount of validation introduced, which should be expanded in future to correct common issues such as missing wheels e.t.c.
Intended usage in Content Packs/Users of the mod
The underlying config reading systems should not be of interest, however improved logging should be of use to pack makers.
Compatibility
I do not anticipate this update will occur configs to be interpreted differently, however there are likely to be issues due to things we've missed. A long list of changed behavior will be amended when this is manually merged.
Where a config which is meant to be set once (like Driver -10 4 59) is specified multiple times, the FIRST usage will be used instead of the LAST.
Wheel positions will be read in a more "generous" manner if they're not properly specified.
Other
WillyJ started this project a few months ago to tidy up the code, implemented the bulk of ConfigMap and ConfigUtils and started to convert the existing type files. However, he soon disappeared and it was left to me to finish conversion and implement error handling and logging. Hence, this took up a lot more of my time than I anticipated.