jpcsupplies / Economy_mod

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

BUG: 2 prescision rounding being used on screen when 3 precision appears to be in back end #52

Closed jpcsupplies closed 8 years ago

jpcsupplies commented 8 years ago

if i have 0.71 iron ingot and attempt to sell 0.71 iron ingot i dont have enough if i try to sell 0.70 it sells fine

(clarification when you open your inventory with "i" in game it rounds the ores to 2 decimals, so a player thinks he needs to type /sell 0.71 "iron ingot" when in fact he probably has 0.709 etc - since players are unlikely to think to mouse over or right click drag to work out the real number - they will see this as a bug.. but since we dont control keens volume rounding its not our fault but we should cater for it)

appears to be a case that we are using there may be 3 or more precision in game back end

suggested fixes: a: round down and trim/truncate players inventory after each sale so what is displayed is actual not rounded. Ie to force 2 precision [ actual = ((integer(qty * 100))/100) ] result = [ qty = 1.123456 actual = 1.12 ] apply actual qty to inventory b: get the /sell all item (all logic) working so a player can /sell all "iron ingot" in which case the precision is irrelevant.

Midspace: option C: Echo back the actual quantity held :) :) :+1: :+1:

jpcsupplies commented 8 years ago

We should be able to adapt this to populate our amount field in the case that a player uses /sell all (at least /sell all ITEM anyway, just /sell all would need a confirmation prompt ;) ) .....

        var inventoryOwnwer = (IMyInventoryOwner)character;
        var inventory = (Sandbox.ModAPI.IMyInventory)inventoryOwnwer.GetInventory(0);
        MyFixedPoint amount = (MyFixedPoint)ItemQuantity;

        if (!inventory.ContainItems(amount, content))......

presumably "amount" relates to the held amount... so - how do we need to change this so we can get out an actual number for the qty held of the specified item? "inventory" i assume is a list of all held items by player using "content" there should be a way to back track how much they actually have ?

midspace commented 8 years ago

I suggest you double check your values. Space Engineers rounds up all the display values.

I can have 0.709 in the save file. In game it displays 0.71

We don't physically have 0.71 to sell.

You can use Admin command to get exact amounts into your inventory. /invadd ingot iron 0.709

jpcsupplies commented 8 years ago

Option B above is a feature needed anyway... so i am looking at working out the /sell all logic as a resolution to this, so that whatever rounding is going on doesnt matter as "all" will populate it anyway.

option A would if used "destroy" small amounts of ores 3 decimals or above.. which seems minor, but on some ores if they get quite expensive thats a lot of money players may throw away, while all protects the qty

midspace commented 8 years ago

See above check in. :grin:

jpcsupplies commented 8 years ago

rofl or option C tell the player how much there is lol

           var storedAmount = inventory.GetItemAmount(content.GetObjectId());
jpcsupplies commented 8 years ago

to get the /sell all BLAH working - Unless you want to beat me to it.. how about under messagesell we add a bool - [ProtoMember(9)] public bool SellAll;

at the start of message sell we add something like: if (SellAll) { itemQuantity=inventory.GetItemAmount(content.GetObjectId()); }

and in economyscript when calling messagesell: MessageSell.SendMessage(buyerName, sellQuantity, content.TypeId.ToString(), content.SubtypeName, sellPrice, useBankBuyPrice, sellToMerchant, offerToMarket,false);

unless of course our split discovers we used /sell all "item name" in which case

MessageSell.SendMessage(buyerName, sellQuantity, content.TypeId.ToString(), content.SubtypeName, sellPrice, useBankBuyPrice, sellToMerchant, offerToMarket,true);

midspace commented 8 years ago

/sell ALL "iron ore" yes. So instead of /sell 5673.34892 "iron ore" when you are trying to figure out how much you have exactly in your inventory to the nth decimal place.

jpcsupplies commented 8 years ago

Exactly - i think my regex failure which you fixed allowed for an "OR string" in our qty field.. so it should be pretty straight forward (i think) to add that

midspace commented 8 years ago

how about under messagesell we add a bool - No.

Why, because it's too easy to allow a passed in command (from some outside source, faking out your SteamId) to remove all your inventory without knowing what is in it. We can check the full inventory amount in the client code before passing it through to the server.

jpcsupplies commented 8 years ago

Ah, Ok so put it somewhere before the call to messagesell; where code is still client side you are saying.. would my split (if all) condition cut it, below - it has a placeholder for "all" contingency currently.. tho most of your prior match.success code mostly overrules that i think

                    default: //must be more than 2
                        //ie /sell all uranium || /sell 1 uranium || /sell 1 uranium 50 || /sell 1 uranium 50 bob to offer 1 uranium to bob for 50
                        //need an item search sub for this bit to compliment the regex and for neatness
                        //if (split[3] == null) split[3]= "NPC";
                        if (split.Length == 3 && ((decimal.TryParse(split[1], out sellQuantity) || split[1].Equals("all", StringComparison.InvariantCultureIgnoreCase))))//eg /sell 3 iron
                        {   //sellqty is now split[1] as decimal -
                            if (sellQuantity == 0 && split[1].Equals("all", StringComparison.InvariantCultureIgnoreCase))
                            {  //in this case try parse failed but "all" matched..
                                //if split[1] = all then we would need to sell every specified item the player is carrying
                                //sellQuantity = TotalPlayerCarriedQuantityOf(match.Groups["item"].Value);
                            }

                            if (string.IsNullOrEmpty(itemName)) { itemName = split[2]; } //assume our regex match failed but we somehow fell here in the split check - maybe be unnecessary
                            //sell price is set by price book in this scenario, and we assume we are selling to the NPC market
                            buyerName = EconomyConsts.NpcMerchantName;
                        }
                        else { MyAPIGateway.Utilities.ShowMessage("SELL", "Debug: qty wasnt valid?"); return false; }
midspace commented 8 years ago

I said all that, and realize we would to fetch the player's character, and dig our their inventory which isn't touched client side. I've working code for this.

jpcsupplies commented 8 years ago

this tested all good in offline mode here can probably close it now.. looks like auto complete is working now too after that regex change which i didnt realize was a bug lol

midspace commented 8 years ago

Auto complete?

jpcsupplies commented 8 years ago

Sorry you would call it pattern matching eg /sell 10 auto vs /sell 10 "Automatic rifle" I took it for granted i had to type it out in full, but it seems to pattern match now. I'd not tried it for a few commits so it may have been fixed for a while..

Ill double check if it pattern matches in quotes too ie /sell 10 "auto" too

yups thats working too Dunno, wasn't working last time i tried it.. now it works everywhere

midspace commented 8 years ago

It's partial string matching in the FindPhysicalParts(..) It tries for an exact match first in the search list. Then starts with that string in the search list. Then contains that string in any part of the search list. Always looking for just 1 match. No more, no less.

jpcsupplies commented 8 years ago

Anyway back to OP - tested on my iron, apparently i had 0.706981 when i did /sell all "iron ingot" so you have fixed this issue and got the ALL to work fine.. close it :) :+1:

midspace commented 8 years ago

You can use "rifle", "auto", "matic rif".

jpcsupplies commented 8 years ago

matic rif too ? cool didnt know that