Reintroduce include statements

[applenick]

Reintroduce include statements

Include statements are back 🎉

This PR reintroduces include statements, which allow for global XML files to be loaded and re-used across different maps. Our new implementation is slightly different from the version used on OCN, but should allow for a very user-friendly and familiar experience.

Example

Let's showcase this awesome feature with a simple example, say you're operating a blitz tournament and want every map to have standardized values. Normally you would be able to achieve this by modifying each individual map to ensure they all have the same values set.

With map includes, it's as simple as first defining your map include file:

<!-- blitz.xml, located under includes/blitz.xml -->
<map>
    <blitz>
        <lives>5</lives>
    </blitz>
</map>  

PGM will automatically infer the include id based on the file name, in this case blitz.xml -> blitz

Then to include the blitz file, you just have to reference it using the following statement.

<!-- map.xml -->
<map>
  ...
  <include id="blitz"/>
  ...
</map>

The benefit is now you'll be able to modify values in a single location and have all maps which reference the include be provided with the same values. Think of all the cool possibilities this feature will unlock 🤯

Quick Facts

• Include statements are stored under a configurable directory (Defined in config.yml at map.includes).
• Any number of include files can be loaded by PGM.
• Multiple include statements per map are supported.
• Include files must be unique in name, if more than one of the same name are loaded only the first will be stored.
• The legacy <include src="file.xml"/> from OCN is NOT supported, but will not prevent the map from loading. Instead a small warning will be shown in the console encouraging server operators to upgrade.

Any and all feedback is welcome! As always, these changes have been heavily tested and work as intended 👍

Signed-off-by: applenick [email protected]

[alexsosnovsky]

Oh wow, this is unexpected and awesome!

[applenick]

@RuedigerLP suggested adding back the global.xml file too, not something I originally considered when implementing this. Though would likely only take a few lines of code to make it functional.

@Electroid is that something you would like me to add into this PR or perhaps at a later date?

I went ahead and implemented it as per @Pablete1234's suggestion.

[Pablete1234]

When i suggested it originally i expected you to just save the root element, not switch it to Document , and then just return root.cloneContent() on getContent method. But i think it's fine either way.

[KingOfSquares]

Will this properly trigger an XML update without a server restart if a map has an include and the include is changed? Looking at SystemMapSourceFactory it looks like it currently only checks for modifications in the map.xml file.

[applenick]

Will this properly trigger an XML update without a server restart if a map has an include and the include is changed? Looking at SystemMapSourceFactory it looks like it currently only checks for modifications in the map.xml file.

Great observation. At the moment there's no way to automatically detect when changes occur in the include files, the MapIncludeProcessor#reload is called alongside PGM's config reload (/pgm). Although this will reload the includes manually, maps which have already had their context loaded in will not have the updated XML due to this line here.

A potential solution for the map context issue would be to have MapSource store a list of includes per map (added on load) and combine some logic so MapSource#checkForUpdates will also check the status of all includes too.

As for checking if XML updates will auto-trigger if the include is modified without running the reload command, unsure of the best solution. If anyone has any suggestions let me know!

[KingOfSquares]

Will this properly trigger an XML update without a server restart if a map has an include and the include is changed? Looking at SystemMapSourceFactory it looks like it currently only checks for modifications in the map.xml file.

Great observation. At the moment there's no way to automatically detect when changes occur in the include files, the MapIncludeProcessor#reload is called alongside PGM's config reload (/pgm). Although this will reload the includes manually, maps which have already had their context loaded in will not have the updated XML due to this line here.

A potential solution for the map context issue would be to have MapSource store a list of includes per map (added on load) and combine some logic so MapSource#checkForUpdates will also check the status of all includes too.

As for checking if XML updates will auto-trigger if the include is modified without running the reload command, unsure of the best solution. If anyone has any suggestions let me know!

What about doing a quick search through the map.xml in SystemMapSourceFactory#getDocument for the include keyword? That could store the found ids and store references to the respective include files as well(which could check for modification similar to how its done on the main document) as you described

[KingOfSquares]

Also, does errors work when the error is from an include? I can imagine it would show as an element in the map.xml

[applenick]

What about doing a quick search through the map.xml in SystemMapSourceFactory#getDocument for the include keyword? That could store the found ids and store references to the respective include files as well(which could check for modification similar to how its done on the main document) as you described

I'm not sure parsing the document in that method is the best place for this, but thanks for the suggestion! I've got another idea I'll try though, will see if I can get a commit pushed today/tomorrow 👍

Also, does errors work when the error is from an include? I can imagine it would show as an element in the map.xml

Excellent question! I've not tested that, but I assume it would show the error as part of the map which should be fine. I'll have to check how OCN handled this to compare.

[applenick]

Sorry forgot to check this PR for a few days, just went through and made Pablo's most recent suggestions along with a rebase. Thanks again! I'm not sure what else can be improved upon for this, but always open to suggestions 👍