[Bug Report] Environment variable settings do not apply if PalWorldSettings.ini does not contain the variable

Callum027 commented 4 months ago

Have you read the Important information text above

[X] Yes i did

Current behavior

When a configuration field is not defined in PalWorldSettings.ini, the container cannot apply any values set using the environment variables, since the sed commands used to apply the settings only do in-place edits of existing fields, not inserts. Since PalWorldSettings.ini already exists, the container doesn't override it.

[/Script/Pal.PalGameWorldSettings]
OptionSettings=(PublicIP="192.0.2.1",PublicPort=8211,AdminPassword="<snip1>",ServerPassword="<snip2>",ServerName="SnippetServer",ExpRate=1.300000,PalCaptureRate=2.000000,PlayerDamageRateAttack=1.500000,PlayerDamageRateDefense=0.700000,CollectionDropRate=2.000000,EnemyDropItemRate=2.000000,DeathPenalty=None,PalEggDefaultHatchingTime=2.000000)

I tried to set RCONEnabled and RCONPort using environment variables, but since they weren't defined in PalWorldSettings.ini they were ignored.

Desired behavior

All values set by environment variables should be actually applied, regardless of the current state of the configuration file.

I think there are two possible solutions:

Always overwrite PalWorldSettings.ini with DefaultPalWorldSettings.ini on startup, and overwrite the defaults with values set in the environment variables.
- This will ensure any configuration parameters managed by the container are always correct, but any manual changes made by the user will be lost.
Reimplement config file management to insert configuration fields into the file if they exist.
- This takes much more work, but would preserve any manual modifications made by the user. Palworld uses default values if fields are not defined in PalWorldSettings.ini, so this would basically be the ideal solution.

Links to screenshots

No response

To Reproduce

Steps to reproduce the behavior:

Remove a configuration field definition from PalWorldSettings.ini.
Try to override the setting using the environment variable.

Software setup

OS: Ubuntu 22.04 LTS
Docker: Docker v25.0.3, Docker Compose v2.24.5

Hardware setup

vCPU: 4 vCPU
RAM: 16GB
Disk: 10GB root volume, 10GB dedicated volume for Palworld (both NVMe high IOPS virtual volumes)

Additional context

I migrated my Palworld Dedicated Server setup over from a non-containerised installation, where in PalWorldSettings.ini I only set the values I wanted to override. This is why the PalWorldSettings.ini file is not a complete copy of the defaults file.

Callum027 commented 4 months ago

I believe this will become a bigger issue in the future if Pocketpair decide to add new configuration values to Palworld in new updates.

jammsen commented 4 months ago

Hey @Callum027

Always overwrite PalWorldSettings.ini with DefaultPalWorldSettings.ini on startup, and overwrite the defaults with values set in the environment variables.

This will ensure any configuration parameters managed by the container are always correct, but any manual changes made by the user will be lost.

This is litteraly what i do, IF the config does not exists, for backwards compability: https://github.com/jammsen/docker-palworld-dedicated-server/blob/develop/includes/config.sh#L45

Reimplement config file management to insert configuration fields into the file if they exist.

This takes much more work, but would preserve any manual modifications made by the user. Palworld uses default values if fields are not defined in PalWorldSettings.ini, so this would basically be the ideal solution.

What do you mean by that? I dont understand what you mean, please elaborate/reforumalte.

Callum027 commented 4 months ago

Always overwrite PalWorldSettings.ini with DefaultPalWorldSettings.ini on startup, and overwrite the defaults with values set in the environment variables.

This will ensure any configuration parameters managed by the container are always correct, but any manual changes made by the user will be lost.

This is litteraly what i do, IF the config does not exists, for backwards compability: https://github.com/jammsen/docker-palworld-dedicated-server/blob/develop/includes/config.sh#L45

If you go a little further up to includes/config.sh:39, there is a condition set that it will only copy the file from DefaultPalWorldSettings.ini if PalWorldSettings.ini does not exist. This is the issue I am hitting: On existing installation the file already exists. So if a configuration parameter you're setting is not defined within the existing file (like in a Palworld version upgrade), it cannot be overwritten.

Reimplement config file management to insert configuration fields into the file if they exist.

This takes much more work, but would preserve any manual modifications made by the user. Palworld uses default values if fields are not defined in PalWorldSettings.ini, so this would basically be the ideal solution.

What do you mean by that? I dont understand what you mean, please elaborate/reforumalte.

So the current sed -E -i commands will not add configuration files to PalWorldSettings.ini if they are not already defined in there. What I mean is figure out a way to reimplement these so that it inserts the configuration field at the end of the file, if it is not already defined.

Here's an example of how it might be done for Difficulty (it can be turned into a function to reduce the amount of duplication):

if grep "Difficulty=[a-zA-Z]*" "$GAME_SETTINGS_FILE" > /dev/null 2>&1; then
    sed -E -i "s/Difficulty=[a-zA-Z]*/Difficulty=$DIFFICULTY/" "$GAME_SETTINGS_FILE"
else
    sed -E -i "s/)$/,Difficulty=$DIFFICULTY)/" "$GAME_SETTINGS_FILE"
fi

jammsen commented 4 months ago

Hey @Callum027

If you go a little further up to includes/config.sh:39, there is a condition set that it will only copy the file from DefaultPalWorldSettings.ini if PalWorldSettings.ini does not exist. This is the issue I am hitting: On existing installation the file already exists. So if a configuration parameter you're setting is not defined within the existing file (like in a Palworld version upgrade), it cannot be overwritten.

Oh so your request is bascially, ALWAYS copy the new default and change it?

So the current sed -E -i commands will not add configuration files to PalWorldSettings.ini if they are not already defined in there. What I mean is figure out a way to reimplement these so that it inserts the configuration field at the end of the file, if it is not already defined.

Here's an example of how it might be done for Difficulty (it can be turned into a function to reduce the amount of duplication):
if grep "Difficulty=[a-zA-Z]*" "$GAME_SETTINGS_FILE" > /dev/null 2>&1; then
    sed -E -i "s/Difficulty=[a-zA-Z]*/Difficulty=$DIFFICULTY/" "$GAME_SETTINGS_FILE"
else
    sed -E -i "s/)$/,Difficulty=$DIFFICULTY)/" "$GAME_SETTINGS_FILE"
fi

I seed the code and see no difference, the values gets overwritten anyways, i dont see a change to what i have. Not sure what you want to do here. Sorry, maybe im blind or dumb, but i dont get it.

Callum027 commented 4 months ago

If you go a little further up to includes/config.sh:39, there is a condition set that it will only copy the file from DefaultPalWorldSettings.ini if PalWorldSettings.ini does not exist. This is the issue I am hitting: On existing installation the file already exists. So if a configuration parameter you're setting is not defined within the existing file (like in a Palworld version upgrade), it cannot be overwritten.

Oh so your request is bascially, ALWAYS copy the new default and change it?

Not necessarily. I just thought this would be the easiest way to solve this issue. The disadvantage is that users cannot modify PalWorldSettings.ini to manually add settings the container does not yet manage.

So the current sed -E -i commands will not add configuration files to PalWorldSettings.ini if they are not already defined in there. What I mean is figure out a way to reimplement these so that it inserts the configuration field at the end of the file, if it is not already defined. Here's an example of how it might be done for Difficulty (it can be turned into a function to reduce the amount of duplication):
if grep "Difficulty=[a-zA-Z]*" "$GAME_SETTINGS_FILE" > /dev/null 2>&1; then
    sed -E -i "s/Difficulty=[a-zA-Z]*/Difficulty=$DIFFICULTY/" "$GAME_SETTINGS_FILE"
else
    sed -E -i "s/)$/,Difficulty=$DIFFICULTY)/" "$GAME_SETTINGS_FILE"
fi
I seed the code and see no difference, the values gets overwritten anyways, i dont see a change to what i have. Not sure what you want to do here. Sorry, maybe im blind or dumb, but i dont get it.

So here I'm proposing an alternative to "ALWAYS copy the new default and change it", as explained above.

Here is the current implementation for the DIFFICULTY setting, as shown in develop:includes/config.sh:50-53:

if [[ -n ${DIFFICULTY+x} ]]; then
    e "> Setting Difficulty to '$DIFFICULTY'"
    sed -E -i "s/Difficulty=[a-zA-Z]*/Difficulty=$DIFFICULTY/" "$GAME_SETTINGS_FILE"
fi

In this alternative solution, I'm proposing we could change it (effectively) to:

if [[ -n ${DIFFICULTY+x} ]]; then
    e "> Setting Difficulty to '$DIFFICULTY'"
    if grep "Difficulty=[a-zA-Z]*" "$GAME_SETTINGS_FILE" > /dev/null 2>&1; then
        # If the `Difficulty` field already exists in `PalWorldSettings.ini`, overwrite it.
        sed -E -i "s/Difficulty=[a-zA-Z]*/Difficulty=$DIFFICULTY/" "$GAME_SETTINGS_FILE"
    else
        # If the `Difficulty` field does not exist in `PalWorldSettings.ini`, add it to the end of the file.
        sed -E -i "s/)$/,Difficulty=$DIFFICULTY)/" "$GAME_SETTINGS_FILE"
    fi
fi

This makes the code more complicated (we can simplify this using functions), but it would fix the issue while also preserving the ability for users to manually add fields to the file.

Callum027 commented 4 months ago

I'm fine with any solution you're happy with, I just wanted to propose some things and hear your thoughts.

jammsen commented 4 months ago

I'm fine with any solution you're happy with, I just wanted to propose some things and hear your thoughts.

Yeah sure im all ears for that.

I added now copy Default to location and sed-search-and-replace.

Your version for modifications in a manual way we have covered, its manual mode, there the script doesnt do anything. I think thats a solid base, both version are supported and the "config.sh" doesnt become a nightmare because of insane complexity.

Im open to later feedback, but lets go with this first.

Callum027 commented 4 months ago

I'm fine with any solution you're happy with, I just wanted to propose some things and hear your thoughts.

Yeah sure im all ears for that.

I added now copy Default to location and sed-search-and-replace.

Your version for modifications in a manual way we have covered, its manual mode, there the script doesnt do anything. I think thats a solid base, both version are supported and the "config.sh" doesnt become a nightmare because of insane complexity.

Im open to later feedback, but lets go with this first.

Sounds reasonable. Thanks for fixing the issue, and thanks for listening to my suggestions, appreciate it.

jammsen / docker-palworld-dedicated-server