jpcsupplies / Economy_mod

Basic Economy System for Space Engineers
13 stars 12 forks source link

Minor Bug: Sell to server pulls price from Economy data not Economy Config #53

Closed jpcsupplies closed 8 years ago

jpcsupplies commented 8 years ago

If i change the price of stone ore in the "Economy config file"- then sell to server, it appears the script retrieves the price from "economy data file" instead.

Issue is minor; since script is using NPC/Player logic as requested - but the NPC retail pricing is meant to be a configurable setting. Worst case scenario we can edit price in data instead of config.. although this breaks our design objective of players only needing to edit config.

Suggested work arounds:
~~A: In our "is NPC" check, add a flag to pull the price out of Config instead of data. Assuming the structures are compatible - we may be able to use string for calling filename; which makes it a simple case of if (npc) { file to read for price = config} else ( file to read for price = data } we need to be careful when writing back however.. this string should be local/private so subsequent file calls dont accidentally read "config" where we expect "Data"~~ - I just realized we dont read on the fly we read at startup see option C instead.

or

~~B: If the Config file exists, overwrite the NPC table pricing/blacklist in data on startup (in this scenario only the prices are relevant; although a server admin may also wish to manually restock item amounts) Downsides: npc supply is reset each start up~~

or

~~C: we read the config pricing from "config file" for the NPC price table on startup - when saving quantity pricing back we may need to check if we are writing a player/territory or the main server NPC then write back appropriately. Downsides: a little bit of redundant code used when NPC trading for read/write stock - may make player territory markets require an in game price managment command later on.. Up side: admins can again manage price/qty/blacklist by only needing to touch the "config" file~~

jpcsupplies commented 8 years ago

I was playing with the idea of making the /sell reset command (pull prices/onhand from economyconfig and overwrite npc market economydata) - i very nearly got it working using some recycled 'obsolete' code from marketmanager.cs but I rolled it back and decided to take a closer look at this bug first.

I am trying to work out what is going on in the code - Looking at the program flow here, as far as I can tell it is actually pulling in the economyconfig.xml on initserver() but never actually using it normally?

I have found that if the main economydata.xml file is missing however, it does appear to use it on creating the data file.

So it may be there is an if exist somewhere shorting out the code on startup. Most likely deliberately so live market data isnt writing back to config to maintain config data integrity.

TO resolve this issue I can see four paths - 1: on save event backup the NPC market pricing and on hand to the config file too. When server is restarting load the config file and overwrite the NPC market data.

2: Keep current behavior and I go ahead and get the reset command working; which basically just triggers a one off overwrite on the economy npc market data in the data file from config file

3: Make the reset command trigger a delete of the data file entirely and rerun all the init processes

4: Make the NPC trader trade out of the config file not the data file

5: Although we record all our NPC stock in the economydata.xml file; we instead read our prices and blacklist status off the economyconfig.xml file at startup, but other than that use only the economydata.xml the rest of the time

1: Pros - it gives us our desired admin can tweak pricing behavior and gives us a backup of the npc market cons - a little bit of data double handling; some complexity, we are writing to a config file that should only be edited by admin, if something goes wrong we break the other behavior settings too as the file will reset to defaults.

2: Pros - pretty simple I just cut paste a few obsolete code blocks and change the input and output files - then add an admin check to fire it off a command, it immediately replenishes all stock supplies depending what is in config cons - its a bit "cheaty" it wont be giving admins the option to maintain existing stock levels, and just tweak the prices, it could also potentially corrupt our data file with duplicates or superseded item codes down the line

3: Pros - Crazy simple, just fire a delete event and reinit. Cons - This would also reset all the registered trade locations, bank balances, seen data, and all buy bids and sell offers - basically its a factory reset just to update pricing

4: Pros - gives us admin can tweak behavior, probably quite simple to do (point a few sections to the other filename) Cons - it makes a config file behave like a data file, our databases get confusing, it edits a config file that should only be edited by admins

5: Pros - admin can again tweak pricing in config, we never write to a config file only admins should be touching, no double handling, other than the on init() read of buy/sell prices we are only ever dealing with the economydata.xml file, should be pretty basic to implement (only need to change our init load event to read the prices (and blacklist status?) into our working data set) Cons - we will still need a "reset" command of some sort if an admin wishes reset the on hand in the NPC market, if we add in-game commands to allow staff to set blacklist status, stock levels or pricing, we will need it to write blacklist and pricing to both config, and data files at the same time.

Personally - Option 5 sounds like the safest option, but Option 1 could require less coding.

What are your thoughts here?

jpcsupplies commented 8 years ago

Ok on initialisation of the economy script can we make it pull over the "config XML" file pricing, and blacklist status and overwrite the NPC price list's pricing and blacklist status that is stored in "data XML" file pricing?

This gives us a config file admins can change, but will need to restart server to apply - which is reasonable.

Eventually If we create a set command to change NPC blacklist/ buy and sell prices it will probably also need to change the price/ in both files.. which means if we are using set for normal players a check if it is NPC or not so normal player price changes dont alter config file prices would be appropriate.

jpcsupplies commented 8 years ago

issue now irrellevant as this function can be perfomed in game now