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

Major update for ECO with AMP #207

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

HarryPlopper91
Copy link

ecometaconfig.json

  • Added 15 new meta config file settings.

ecoconfig.json

  • Set default game settings as originally designed by the game devs.
  • Updated description fields to match eco server ui.
  • Added placeholder defaults to help players identify default settings easily.
  • Added 1,123 additional lines of config settings to allow usage and setting options with AMP.

eco.kvp

  • Updated support live setting changes from false to true
  • Added 15 additional config file generation settings.
  • Added HarryPlopper to template authors.

Tested on both Windows and Linux versions of AMP

ecometaconfig.json
* Added 15 new meta config file settings.
ecoconfig.json
* set default game settings as originally designed by the game devs.
* updated description fields to match eco server ui.
* Added place holder defaults to help players identify default settings easily.
* Added 1,123 additional lines of config settings to allow usage and setting options with AMP.
eco.kvp
* Updated support live setting changes from false to true
* Added 15 additional config file generation settings.
* Added HarryPlopper to template authors.
@Greelan
Copy link
Collaborator

Greelan commented Oct 3, 2022

Nice. Will check it out. Why have the two IP binding settings been removed?

Have you confirmed that the live setting update format set <key> "<value>" works in the AMP console?

@IceOfWraith
Copy link
Collaborator

First of all, thank you for taking this on! I know it can be very tedious. There seems to be some extra files in the PR though.

@IceOfWraith IceOfWraith added enhancement New feature or request Windows For applications that run on Windows Linux For applications that run on Linux labels Oct 3, 2022
@Greelan
Copy link
Collaborator

Greelan commented Oct 3, 2022

Also, looks like the space in the CommandLineDelimiter has been inadvertently removed? Probably not a real issue if IncludeInCommandLine and FormattedArgs are not used but should be retained for completeness.

@HarryPlopper91
Copy link
Author

First of all, thank you for taking this on! I know it can be very tedious. There seems to be some extra files in the PR though.

Yeah those are auto gen files from the IDE I use, I suppose, they don't have any effect on the ECO config files :)

@HarryPlopper91
Copy link
Author

Also, looks like the space in the CommandLineDelimiter has been inadvertently removed? Probably not a real issue if IncludeInCommandLine and FormattedArgs are not used but should be retained for completeness.

Good catch, it was not in the original config files (that I remember anyways) when I first started working on them some time ago, but if its needed for other reasons, it won't hurt to have it there 💯

@IceOfWraith
Copy link
Collaborator

First of all, thank you for taking this on! I know it can be very tedious. There seems to be some extra files in the PR though.

Yeah those are auto gen files from the IDE I use, I suppose, they don't have any effect on the ECO config files :)

Yeah, they'll need removed before merging though or they'll be in the CC repo. :)

@HarryPlopper91
Copy link
Author

Nice. Will check it out. Why have the two IP binding settings been removed?

Have you confirmed that the live setting update format set <key> "<value>" works in the AMP console?

Will change live setting update back, I did not confirm it in the AMP console, it does generate an unknown command error.
For the removal of the two IP bindings, good catch there, I was testing something a few weeks back and forgot to add those back in.

I will make the mentioned changes 👍

@Greelan
Copy link
Collaborator

Greelan commented Oct 3, 2022

I mean, for the live settings change, great if the server supports it but I don't know of a server yet that does. The idea (I believe) is that if a setting is changed in the AMP UI, it is not only applied to the config file but also passed to the server process via the relevant command format to dynamically update the running server

changed live updates back to false
added ip bindings that were removed for testing and not put back in.
Copy link
Author

@HarryPlopper91 HarryPlopper91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended changes as requested.

@HarryPlopper91
Copy link
Author

I mean, for the live settings change, great if the server supports it but I don't know of a server yet that does. The idea (I believe) is that if a setting is changed in the AMP UI, it is not only applied to the config file but also passed to the server process via the relevant command format to dynamically update the running server

you are correct, however ECO does not currently support that, it does appear to read the config changes as they happen to the config files minus a few settings that are mentioned need a restart to the game server.

Copy link
Author

@HarryPlopper91 HarryPlopper91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space added back to App.CommandLineParameterDelimiter

@IceOfWraith
Copy link
Collaborator

One more suggestion before a full review is done... Add Eco - before all the categories to help group them and make them stand out amongst the AMP tabs. Greelan and I have started doing this and it helps clean up the menu at least a little.

@HarryPlopper91
Copy link
Author

One more suggestion before a full review is done... Add Eco - before all the categories to help group them and make them stand out amongst the AMP tabs. Greelan and I have started doing this and it helps clean up the menu at least a little.

Some of the added options add to the AMP tabs as they currently are. Is this something that is a requirement now? That would be alot of resorting options, and add additional tabs to take those settings out of the AMP tabs.

@IceOfWraith
Copy link
Collaborator

Not at all. Like I said, just a suggestion! The AMP settings would stay in the AMP tabs. You would just append the Eco - to the beginning of the custom menu tabs that relate solely to Eco. I've updated a big one before by just doing a find/replace on the category names.

Copy link
Author

@HarryPlopper91 HarryPlopper91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended changes to ecoconfig.json

Copy link
Author

@HarryPlopper91 HarryPlopper91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated text formatting from ECO to Eco

@Greelan
Copy link
Collaborator

Greelan commented Oct 8, 2022

Amazing job on adding all these settings! As mentioned on Discord, I've got a few suggested tweaks that are here:

https://github.com/Greelan/AMPTemplates/blob/dev/eco.kvp
https://github.com/Greelan/AMPTemplates/blob/dev/ecoconfig.json
https://github.com/Greelan/AMPTemplates/blob/dev/ecoupdates.json

@Greelan
Copy link
Collaborator

Greelan commented Oct 8, 2022

Just one other thing - I couldn't find the file that has the PercentOfDaysToBeActiveForLongTermDemographic setting?

@HarryPlopper91
Copy link
Author

Just one other thing - I couldn't find the file that has the PercentOfDaysToBeActiveForLongTermDemographic setting?

I just looked at their server ui, and it is a setting that gets added when you set it with their UI but it is not in the civics.eco.template file by default. I'm going to remove it for now, I thought I had got all of those but one got past me lol.

Copy link
Author

@HarryPlopper91 HarryPlopper91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes from suggestions.

@Greelan
Copy link
Collaborator

Greelan commented Oct 9, 2022

I was doing some further review of the settings. Unless I am missing it, I also couldn’t find:

NewGameTemplate
SLGAccountName
SLGAccountPassword
TimeOfDayScale
TimeMult

There may be others too - I haven’t got through checking off all the settings.

Maybe you should do another pass.

Also, the update stage for WorldGenerator.eco adds two .eco extensions. xD

@HarryPlopper91
Copy link
Author

I was doing some further review of the settings. Unless I am missing it, I also couldn’t find:

NewGameTemplate
SLGAccountName
SLGAccountPassword
TimeOfDayScale
TimeMult

There may be others too - I haven’t got through checking off all the settings.

Maybe you should do another pass.

Also, the update stage for WorldGenerator.eco adds two .eco extensions. xD

Interesting, pause on this for a bit, I'm going to pregen new templates on github that have all the correct fields and parameters that the server ui adds that are not apart of the default *.eco.template files, then have Amp do a github pull for them.

Should I just have those on my own GitHub or include a folder with them in it when I do the next pull request update 🤔? Might be the best idea to have them pulled from the Amp repo when the eco pull requests is ready to be accepted as well.

@IceOfWraith
Copy link
Collaborator

That's a completely normal scenario. Look at the update stages for Conan Exiles for an example of downloading a config and renaming the file at the same time. The way it works is you put the files in your PR, but temporarily point the update stages back towards your repo. When it's fully tested and works/approved then you'll change the update stages to match the CubeCoders repo.

@HarryPlopper91
Copy link
Author

That's a completely normal scenario. Look at the update stages for Conan Exiles for an example of downloading a config and renaming the file at the same time. The way it works is you put the files in your PR, but temporarily point the update stages back towards your repo. When it's fully tested and works/approved then you'll change the update stages to match the CubeCoders repo.

Sounds good I'll work on that tonight.

@Greelan Greelan marked this pull request as draft October 23, 2022 07:43
@IceOfWraith
Copy link
Collaborator

No pressure at all, but wanted to check in. Is there anything you're stuck on that we can help with?

@HarryPlopper91
Copy link
Author

Thanks @IceOfWraith, I have had a busy year and had to put a lot of things to the side. Planning on picking back up on this very soon! Thanks for moving it Application Templates :)

@IceOfWraith
Copy link
Collaborator

No stress! I was doing some clean up for PRs that have sat with no movement for a while. If you get back into doing it that's great, if you don't have time I'm sure someone will be willing to finish it eventually here.

@IceOfWraith
Copy link
Collaborator

I'd like to see this one get finished up. @HarryPlopper91 do you want to take a stab at cleaning it up and merging the changes that have been made since this PR was created? If not, I have no issue taking it on.

@HarryPlopper91
Copy link
Author

I should actually have some time over these next few days, I'll dig into again. If I dont push or comment by Monday then please feel free to add on to what I've done so far, I'm always happy to share the dev love.

@HarryPlopper91
Copy link
Author

@IceOfWraith I'm in Florida and recovering from this last hurricane, not sure if I'll find time to complete the ECO template, if you want to take a crack at it, you're more than welcomed to :)

@IceOfWraith
Copy link
Collaborator

Definitely. I'm gonna be neck deep in templates this weekend anyways it seems. Lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Linux For applications that run on Linux Windows For applications that run on Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants