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

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

Closed
1 task done
Callum027 opened this issue Feb 12, 2024 · 8 comments
Assignees
Labels
bug Something isn't working issue-unclear Issue unclear needs-more-information Not enough information provided to help waiting-for-feedback Waiting on feedback

Comments

@Callum027
Copy link
Contributor

Have you read the Important information text above

  • 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:

  1. Remove a configuration field definition from PalWorldSettings.ini.
  2. 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 Callum027 added the bug Something isn't working label Feb 12, 2024
@Callum027
Copy link
Contributor Author

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 jammsen self-assigned this Feb 12, 2024
@jammsen jammsen added issue-unclear Issue unclear waiting-for-feedback Waiting on feedback needs-more-information Not enough information provided to help labels Feb 12, 2024
@jammsen
Copy link
Owner

jammsen commented Feb 12, 2024

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

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

jammsen commented Feb 12, 2024

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

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

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

@jammsen
Copy link
Owner

jammsen commented Feb 13, 2024

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working issue-unclear Issue unclear needs-more-information Not enough information provided to help waiting-for-feedback Waiting on feedback
Projects
None yet
Development

No branches or pull requests

2 participants