Prevent file overrides

[AzzkiyOne]

Prevents Anomaly\Patches\DamageDefs\Damages_Misc.xml from overriding Biotech\Patches\DamageDefs\Damages_Misc.xml when both DLC's are loaded, as well as guards against such issues in the future.

[SamaelGray]

Is there a good reason behind nesting the base CE defs into a "CombatExtended" folder?
The vibe I'm getting from these changes is just making the work on files more tedious, more clicks for very minor gains. I've not profiled the load times with a full mod list but I feel that it's not changed much from the old set up. For the debugging side of things it was a good change, not too sure about the hassle that comes with it.

[AzzkiyOne]

Is there a good reason behind nesting the base CE defs into a "CombatExtended" folder?

This is to avoid potential filename collisions in the future.

Filename collisions can create bugs that are hard to catch. For example, right now Biotech\Patches\DamageDefs\Damages_Misc.xml isn't loading when both Biotech and Anomaly are loaded, because it is being replaced by Anomaly\Patches\DamageDefs\Damages_Misc.xml. There is no error on startup. And judging by the contents of this file, its absence will not be immediately noticeable at runtime.

The vibe I'm getting from these changes is just making the work on files more tedious, more clicks for very minor gains. I've not profiled the load times with a full mod list but I feel that it's not changed much from the old set up. For the debugging side of things it was a good change, not too sure about the hassle that comes with it.

Here are some maybe not immediately obvious pros:

• We can now use proper defs instead of adding them through PatchOperationAdd.
• PackageId is more stable than mod name.
• Previously there were some bugs where some patches wouldn't load because of a typo in mod name inside PatchOperationFindMod. Now, if you make a typo in packageId nothing will load and it would be immediately noticeable.
• Less nesting inside *.xml files.
• Previously most patch operations were nested inside PatchOperationSequence. So if one patch operation fail, the Sequence stops and will not run any additional PatchOperations. One can argue that this is a good thing, because it creates a situation where this failed patch operation cannot be simply ignored (because it affects other patch operations and therefore makes itself more prominent). To which i would argue that the ability to ignore some errors (that is provided by the fact that now errors in patch operations are narrower in scope) is benefitial to the end user and makes the mod more robust.
• We can now control the loading order of mod patches without needing to rename directories.

[masakitenchi]

From what I could tell from Verse.DirectXmlLoader.XmlAssetsInModFolder, nesting DLC folders won't solve the problem if you didn't include the filename change.
For example, given an item in loadfolders like
<li>Anomaly</li>, it will be parsed into ${Path_Until_Mods_Folder}/${ModRootFolder}/Anomaly
(Code in Verse.ModContentPack.InitLoadFolders),
and when the game is trying to load all xml files, it passes the instance of ModContentPack and (Defs or Patches) to XmlAssetsInModFolder, where it combines all folders with the latter argument (Defs or Patches in this case) to make a list of all folders to load, and in the case I provided, it would be

• ${Path_Until_Mods_Folder}/${ModRootFolder}/Anomaly/Defs
• ${Path_Until_Mods_Folder}/${ModRootFolder}/Anomaly/Patches.

Yes, it does remove everything before Defs or Patches when adding it to the dictionary
(so files in ${ModName}/LoadFolder1/Defs/ThingDef/ThingDef1.xml and ${ModName}/LoadFolder2/Defs/ThingDef/ThingDef1.xml has a collision in this case, both will be Defs/ThingDef/ThingDef1.xml when game determines their key),
but since no file system (at least what I know of) allows different file in identical paths,
renaming like ${ModName}/XXX/Defs/{YYY => CombatExtended/YYY}/ZZZ.xml looks redundant to me.
There will be no case of collision under the same Defs or Patches folder.
It should occur when and only when XXX is different, but all paths after XXX is identical.

Conclusion:
I personally agree with changing the file name, but disagree with nesting folders.
Instead, I would like to provide a transpiler that when a collision is detected, it will make the game print an error message.

Tested the transpiler, it does work, but the problem is it cannot tell unexpected collisions from expected collisions

P.S.
On a second thought, it is a simpler way of preventing load issues.

When you have both ${ModName}/Defs/ThingDef/a.xml and ${ModName}/1.5/Defs/ThingDef/a.xml, it does prevent the game from loading the "Common" file. (Given that you put <li>1.5</li> after <li>/</li>)

The question is, we expect loadfolders.xml can tell the difference between "load this firstly, that secondly if it's not loaded before" and "if mod x is loaded, load everything in y folder", yet it doesn't at the moment. if we (or tynan) can make loadfolders.xml be able to tell the difference, life will be a lot easier.

[AzzkiyOne]

@masakitenchi I think you have missed something.

Before

Anomaly
\_Defs
  \_FooBar.xml
Biotech
\_Defs
  \_FooBar.xml
...

LoadFolders.xml:

<li IfModActive="Ludeon.Rimworld.Biotech">Biotech</li>
<li IfModActive="Ludeon.Rimworld.Anomaly">Anomaly</li>

Anomaly/Defs/FooBar.xml overrides Biotech/Defs/FooBar.xml.

After

DLC
|-Anomaly
| \_Defs
|   \_Anomaly
|     \_FooBar.xml
|-Biotech
| \_Defs
|   \_Biotech
|     \_FooBar.xml
...

LoadFolders.xml:

<li IfModActive="Ludeon.Rimworld.Biotech">DLC/Biotech</li>
<li IfModActive="Ludeon.Rimworld.Anomaly">DLC/Anomaly</li>

No collision.

[masakitenchi]

@dmitry-agapov

That is exactly what I have said.

From what I could tell from Verse.DirectXmlLoader.XmlAssetsInModFolder, nesting DLC folders won't solve the problem if you didn't include the filename change.

I need to clarify that "filename change" here refers to any change occurs in the full path after what's written in <li>LoadPath</li>.

In some text editors (i.e. VSCode) it does skip empty folders so that you can look into the nesting folders with one click, but as Samuel said, it still adds an extra layer of complexity.

So, my suggestion would be adding prefix or postfix to the file itself, rather than nesting them in an extra folder.
Same as the defName change (adding CE_ to all CE ThingDefs)

[AzzkiyOne]

That is exactly what I have said.
I need to clarify that "filename change" here refers to any change occurs in the full path after what's written in <li>LoadPath</li>.

Now i get it. I thought that you were saying that this kind of nesting Foo/Defs/Foo/ThingDef/Bar.xml wouldn't work without also renaming the file itself (Foo/Defs/Foo/ThingDef/Foo_Bar.xml), because to me filename is this part: Bar.

So, my suggestion would be adding prefix or postfix to the file itself, rather than nesting them in an extra folder.
Same as the defName change (adding CE_ to all CE ThingDefs)

The rationale behind using a subdirectory approach is that it is less error prone.

[AzzkiyOne]

I think this PR has become irrelevant since #3330 was merged. So i'm closing it.