Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

In-Game Trade Mode/Crafting Shop Menu #25

Closed
codergreen opened this Issue · 75 comments

5 participants

@codergreen

I plan on creating trading and crafting for in the game. I plan on using shop_trade.cpp and shop_trade.h as a starting point if it's OK. There are still a few questions I have about how it should be implemented. Should trading be trading anything (within reason) the player has for anything the shop owner has (with a gold price to make up the difference in value) or should trades be pre-made (for example the shop owner will give you a sword for three potions and some money) and possibly limited? If the second choice, should crafting be a form of trading? Any comments and feedback will be appreciated.

@Bertram25
Owner

Hi @codergreen ,

Thanks for opening this issue.
I'd like to have both the recipe form and the fixed trading form possible within the same menu, even if not permitted at the same time within the game.

  • The recipe form is the free form, you put whatever you want (usually, no money is included in that case), the stuff put is lost and you get something in return.
  • The fixed form, which will be used I suppose more early in the game as a special shop part, will be used to get special items.
  • Note that the recipe form might be independent from any shopping activities.

As for the code starting point, shop_trade.{cpp,h} is fine to me as I must say I'v let them there on purpose. ;)

I'm also open to comments and feedback, if something is unclear or seem not logical.

Best regards,

@Bertram25
Owner

Ah, btw, note that I won't force you to implement everything before reviewing any part of your work. You can work the way you like and put commits little pieces per little pieces as you see fit.
The only thing I request is that the compilation must remain error and warning free, that the code made is of the same style than the rest (yeah, that can sound obvious, but you'd be surprised.) and that you don't open unfinished stuff within the game (there are debug menus for that) so that the players can't fall on them and complain.

@codergreen

I've made some progress but I've also run into a few problems. The way I started was by copying and pasting code from buy.{cpp,h}, and editing it to work basically as a clone of the buy function. I got that to work, so then I edited it so that _trade_price was std::vector instead of uint32 (there are some problems that come with this; I listed them below). Like I though, the game compiled, ran, and then crashed because nothing was in the vector or because I added no trades. So I looked into adding a trade in items.lua and editing global_object.{cpp,h}. I changed my mind about _trade_price, so right now it is std::vector, where uint32 is the id number of the ShopObject. My first question is how do I convert an id number to a ShopObject? Also I'm not very familiar with git or github. How do I commit pieces to github?

As I mentioned above, when I first get the trading "working" it will have a few problems I will need to patch. One of the problems I would need to fix is that the price is displayed as pictures and names of the items you're giving in the trade all stacked up on each other. Also you are currently unable to pay money in trades and you can only receive one item. Another problem, all items must have one trade price and only one trade price. There are most likely some problems I didn't mention and I could probably patch these problems, but I wanted to know if think there is a better way to do this.

Some more questions: What do you want the user interface to look like? Should I list if an item can be traded for more than once or how many of the item you already have?

Edit: I noticed it all looked like one block of text

@Bertram25
Owner

Hi @codergreen

Nice to see you hopped on work ;)

My first question is how do I convert an id number to a ShopObject?

You can do something like this for instance:
https://github.com/Bertram25/ValyriaTear/blob/master/src/modes/shop/shop.cpp#L1638

How do I commit pieces to github?

Well, first, make sure you made your own fork out of my repository. If you're unsure, just read github doc about forking.
Then git clone it. (using your URL not mine. I.E.: git clone git@github.com:codergreen/ValyriaTear.git)
then cd into it, and make your modifications there.
Once done and okay with it, type:
git commit -a
An editor will open so you can type the commit message, save and the commit is ready.
If you want to make it viewable to everyone. You'll have to do: git push
To stay in sync with my own repository, you'll need to pull from it, using:
git pull --rebase git://github.com/Bertram25/ValyriaTear.git
The rebase option means your unmerged committed patches will stay on top of your own git history.

Try also to read git doc about interactive rebasing (useful to merge commits and redo their order or wording), diff squash handling (for when you've got unfinished stuff you don't want to commit and can't resync), and conflicts handling, those will come handy.
Don't hesitate to make tries in your own repository and too destroy it and refork if you come into something you don't want to handle (at least until you've got patches for me to review there.)

Best regards,

@Bertram25
Owner

Some more questions: What do you want the user interface to look like?

Good question. If you give me a bit of time, I can come up with something, but maybe you had your own idea?

Should I list if an item can be traded for more than once or how many of the item you already have?

Don't bother with that. As long as the player has got the requirements, he/she should be able to obtain it.
Key crafted items will appear or not based on story events.

@codergreen

I've made some more progress and we might not have to worry about some of the problems I listed above. For example you don't have to add trading to every item. Thanks for the code, by the way; I think that's what I need. To test the trading, I'm currently using the buy button, so buying is disabled in my version for now; I can't push anything to github yet. I can create multiple trade listings right now, but you can't see the price yet. Also, for some reason I can't get the shop to accept trades (can't buy). Either I coded something wrong or I don't have the necessary items (I'll check into both).
So my question: is there a trade tab for the shop I could add?

@Bertram25
Owner

Hi,

Is there a trade tab for the shop I could add?

Yes, definitely. :) Add the trade option back, I'd say between Sell and Confirm.
The trade option shall be disabled or even better, not appear when there is no trades. ;)
(I know making it disappear is an additional difficulty, so just make disabled for now when there is no trades if you want.)

Also, it's a good thing that you focused on the trade mode only for now. It's definitely the one missing.
The recipe form should independant from the shop mode anyway.

Best regards,

@codergreen

I haven't accomplished much yesterday or today but I'm still working on the code. A problem you should know about is that there can only be three tabs for the shop at one time and there are five buttons (I didn't know there was a leave shop button before). You're still able to buy and sell stuff, but you have to press a key to exit instead choosing the confirm tab.
The trade tab already becomes disabled when there are no trades (it will crash otherwise). I'll work on removing the trade tab when there are no trades after I get the basics of trading working.
I figured I'd also mention that I saw that you wanted no tabs and it's not a problem for me.

@Bertram25
Owner

Hi @codergreen
Thanks for the news! :)

A problem you should know about is that there can only be three tabs for the shop at one time and there are five buttons (I didn't know there was a leave shop button before).

I'm not sure what you're exactly talking about, but there is likely dead code in that game mode. I don't see the problem of adding a tab at all, so I guess I didn't get the point.

Regards,

@codergreen

I didn't understand where the code for adding a button was but I think I do now, so never mind what I said.

I've almost got the basics of trading done. I just need to fix a problem with taking items out of the inventory, then make sure there are no memory leaks, and finally remove the tabs and replace them with 4 spaces. There will still be a few things I will still need to add. One problem is if you exit the shop menu and enter it again, the shop restocks, including trades. I'll look into how to fix it for trading, but what do you want me to do about the shop instant restocking stuff you can buy?

@Bertram25
Owner

Hi,

Congrats for all the progress made.

I saw no buttons in the code, but rather option boxes. I guess you'll have to make the option box list have 3 or 4 items based on whether the shop has got trade or not, IMHO.

One problem is if you exit the shop menu and enter it again, the shop restocks, including trades. I'll look into how to fix it for trading, but what do you want me to do about the shop instant restocking stuff you can buy?

This can be dealt with at the map script level. Something like: If this key item is bought, don't add it again.
Or, we can have a "traded_items" table in the save informations (See Global manager::Group Events) about the number of traded items for each item id and deal with that, but let's not go too fast.
I'd like to see what you've got so far, so that it can be reviewed and included without being too big to read and understand. ;)

Afterwards, and a bit of in game try of this, we'll be able to discuss it further, won't we?

Best regards,

@codergreen

I've got some news: I won't be able to help for somewhere around 7 to 10 days because I won't have electricity. Because of this, I pushed my work on github, so it should be online. The code didn't throw any errors when I compiled it, but it did have warnings about unused variables. I didn't get a chance to fix the things I listed above, but as long as trading is disabled you shouldn't have any problems in the game.

@Bertram25
Owner
@Bertram25
Owner

Hi again,
I need to test it thoroughly, but the mix between tabs and spaces makes it indeed hard to read for now. I'll review only the shop_trade.{h,cpp} part for now. Thanks for the work done. :)

@Bertram25
Owner

Re,
I've rechanged the tabs into spaces and pushed a squashed commit of your work and mine here, in the trade-test branch of my repository:
bd74790

I'll use this to make a review.

@rootslinux

I'm late to the discussion here, but I wanted to elaborate on what I had intended the trade feature to do in Allacrost (it's fine if VT does something completely different like you've talked about here). I had proposed the trade menu in Allacrost as a means of simplifying a very common transaction in many RPGs:

1) Buy new equipment from shop
2) Leave shop, enter character menu
3) Replace old equipment with new equipment on the characters
4) Re-enter shop and sell old equipment

This requires a lot of steps that I felt were completely unnecessary. You also have a problem when you can't afford to buy all of your new equipment until you sell some of your old equipment. So you might repeat the above steps a few times as you buy new equipment, sell old equipment, buy more new equipment with your sale money, etc. It's inefficient.

In one sentence, here's what the trade menu is supposed to do:

Allow the player to instantly replace their current equipment with new equipment purchased from a shop in a single transaction.

Here's an example. Let's imagine Bronann has the Wooden Sword equipped and the party has 120 drunes. The player enters a shop and finds an Iron Sword for sale for 100 drunes. The player wishes to upgrade Bronann's weapon to this new sword, so he/she enters the trade menu, selects the character to trade (Bronann), selects the "Weapon" equipment slot, then selects the Iron Sword to replace the currently equipped Wooden Sword. The Wooden Sword has a sell price of 40 drunes, so the selected transaction would change the player's drune count by: 120 - 100 + 40 = 60 drunes.

Does that make sense? Here's another example to illustrate a cool feature of the trade system.

Let's now say that we want to make the following trade: Kalya's Wooden Sandals traded for Leather Boots. The party has 80 drunes, the sandals sell for 20 drunes, and the boots cost 85 drunes. With the trade system, it can determine that the player can afford the boots even though they do not have enough drunes to outright buy the boots. The trade system sees that the transaction would result in: 80 - 85 + 20 = 15 drunes, a positive amount so the transaction is valid. If this type of trade was not supported, then the player would have to do the following to get the boots: 1) unequip the sandals from Kalya, 2) Sell the sandals at the shop, 3) Buy the boots, 4) equip the boots on Kalya.


So to re-iterate, the whole purpose of the trade system is to achieve the same result as a simple buy/sell system would in a faster and more efficient manner. The player spends less time in menus and more time playing the game as a result. :) I don't know if this is something that Bertram would approve of, but that's what I always intended that feature to be.

@Bertram25
Owner

Hi again @rootslinux ,

It's quite a good thing to get feedback about the birth of certain features. Please keep up commenting that way when necessary. :)

Frantically, I must say I've always be having mixed feeling about the shop mode. As for me, and many testers (among several in my own family) find the current shop mode very complicated, whatever the need is, being RPG players or not.
Ok, they fiddle a bit and they find out how it works, but yet, nobody out there is convinced about the overall intuitivity of that interface, me neither. Remember I don't blame anybody and I'm not telling I've got all the answers.

The philosophy I want to push here is that whatever we do, we should follow strictly what the user expects to see and stay simple and clean.

Note that in a first place that you considered adding a trade mode because the buy/sell/confirm menus are taking too much time to deal with. I can understand that, but I'd rather simplify the buy/sell menu and get rid of the confirm mode, so that the player can do those basic steps very quickly.
Just look at what other games do, and I'll take final fantasy as an example, IIRC they've never gone further than proposing to equip the armor you've just bought. The buy/sell menu are always simplistic, even if FF12.
Last thing I'll say is that I know that people (and I know why) are usually excited when coming in a shop because they came several times earlier to look at their next buy, remembering the price, making plans to get the money. Correct me if I'm wrong, but from what I saw, they usually don't care about entering the buy/sell menus several times and are excited in doing a bit of micro-management to shape up the perfect equipment, since after all, if the shop is intuitive, it is done quick quickly and only once in a while.

I'm of course open to comments also. But IMHO, what the game actually needs, is to simplify the buy sell mode, and add the trade mode, as it was defined between @codergreen and me.

Best regards,

@rootslinux

That makes perfect sense. And yes, I agree that in it's current state, the shop mode is not at all intuitive. I feel like it needs a tutorial similar to battle mode (which is also not terribly intuitive with all of its new concepts of attack points, status effect intensities, etc). Do what you feel is best for your project right now and you'll hear no complaints from me. I was merely sharing what the original intent was with the trade system when I proposed it a while back for Allacrost. It's perfectly alright if Allacrost and VT do things differently. In fact, I think it's a great thing if they have major differences like this. It's more useful code for other people to utilize in their own projects, and it will also help the two games feel more distinct from one another if they don't play exactly the same way.

@Bertram25
Owner

Good to hear :)

Still looking forward to play Allacrost, btw.

@codergreen

Sorry for the silence for about a week. I was procrastinating and having trouble with the code, but now I've made some progress. I'll rebase git and push my work soon. I made it so that trading removes the items from inventory, but I have yet to add quantity and to fix the problem where you can sell all your items and trade the same items at the same time.
I'm looking forward to Allacrost too. :)

@Bertram25
Owner

Hi @codergreen :)

It's good to hear news from you. Np, take the time needed.

@codergreen

Ok I ran into two problems right now. I decided to reinstall Windows on my computer since I play computer games a lot. The first problem is that my git repository won't rebase. The second is that CodeBlocks compiles, but when the program runs it says I'm missing libpng12-0.dll.

@Bertram25
Owner

Hi @codergreen,

You have to copy the dll files from the lib directory into the same folder than the binary to let it run.
Rename libpng.dll is necessary.

As for the can't rebase, maybe you should try to push without rebasing and I'll do the rest for you.

@codergreen

OK changing the name of the dll helped, thank you. I'll push something as soon as I get something done.

@shirishag75

Oh wow, wonder how I missed this so far. As a player I have few complaints :-

The shop mode is quite unintuitive for me. What would make it easier if it could be doing by keyboard instead of the mouse. I feel confirming the transaction in another tab makes no sense. It could be just another dialog box with options and be done with it.

Confirm
a. yes
b. No.
c. Modify

OR

Confirm
a. yes b.No. c. Modify

After the transaction is known to be valid and you are done with the shopping. That third tab should be dropped off IMHO. It is just cluttering the interface.

I do like the way @rootslinux has shared, that actually makes more sense but it's upto you.

@Bertram25
Owner

What would make it easier if it could be doing by keyboard instead of the mouse.

???

As for the rest, it's an idea. But let's talk about the shop mode once the release is out and when I have a complete proposal.

@Bertram25
Owner

@codergreen Ok, looking forward to it. :)

@codergreen

I got the trading to partially work but it is still missing a few things. One problem I ran into though is that when you trade all of any type of item, the game crashes. I think it has to do with the way I used RemoveObjectToSell (accidently labeled RemoveObjectToTrade as problematic code).
I have a question. Should the way the shop work be changed before I continue? It might make a problem where you can trade and sell an item at the same time go away (although if trading is only going to involve key items, this won't be a problem).
Almost forgot to mention that I pushed my code to github :)

@Bertram25
Owner

Hi @codergreen and Merry Christmas even if a bit late :)

Thanks for the job done. I'll have a go at trying your code asap and try to answer your questions asap :)

Enjoy!

@Bertram25
Owner

Hi @codergreen ,

I got the trading to partially work but it is still missing a few things. One problem I ran into though is that when you trade all of any type of item, the game crashes. I think it has to do with the way I used RemoveObjectToSell (accidently labeled RemoveObjectToTrade as problematic code).

Hmm, yep. I don't know what's the origin of it but I don't want to step on your toes if you think you can handle it.
Tell me if you need help.

I have a question. Should the way the shop work be changed before I continue? It might make a problem where you can trade and sell an item at the same time go away (although if trading is only going to involve key items, this won't be a problem).

IMHO, and to be completely honest, I do think that indeed the shop menu should be simplified first since you're plainly struggling with bugs caused by two diverging designs.
I do think that either:

  • We should get rid of the confirm part right now, or:
  • Have two modes for the shop menu as an half-step forward, I mean: Permit to run the shop as a trade interface or as a buy/sell one, but not the two at once. (which would be case when get rid of the confirm interface.)

What would be the most feasible part for you?

Almost forgot to mention that I pushed my code to github :)

Yes! And I reviewed the part seen so far.

Thanks a lot for the job done and best regards,

@codergreen

I could try and see if I can get rid of the confirm menu. That way sounds easier (I hope it is). Also after I finished that and I have a more complete trading system, I could try and allow trading by itself, if you don't want someone to run a full shop.

Do you know how to get codeblocks to work in debug mode? Lua seems to throw errors when I try. If not, is there a good way to trace errors? I did something to get gdb to work on linux, but I didn't know if you knew of a better way. I think I saw somewhere that you mention mingw, so I got the codeblocks that download that included it but I don't know how to start it.
Thanks for the help.

@Bertram25
Owner

Hi @codergreen

MingW is gcc for windows in short, so if you're compiling successfully, you should already be using gcc all that simply.

to debug your program, the simplest and best way (both on linux and windows btw) is to run the program in gdb directly:

gdb src/valyriatear from the program root directory in a terminal.

Then type 'run' from the gdb interface and play. Once it crashes, type 'bt' (aka backtrace) in the gdb terminal interface to get the traces of the crash. To make sure that valyria is compiled with debugging symbols (permitting) to get the exact line of code of the crashing function, for instance, make sure to use the '-g' flag in the build options of C::B.

I hope it helped. Feel free to keep asking questions if you need to.

@codergreen

@Bertram25
Thanks for the help; i got gdb up and running. I started working on removing the need for the confirm menu and so far, so good. A few small changes to the code seem to have been all that is needed so far, although i need to fix a bug that I created. It crashes the game when you sell everything in your inventory, but I don't think it should be too much trouble.

@Bertram25
Owner

Great to see I help :)

I'm looking forward the new codergreen's shop mode.

@codergreen

I finished fixing the bug somewhat. I got it working by returning to the root menu whenever a sale is made. I can change it so that it only goes to the root menu after everything is sold instead of after every single sale if you want, it will take a little more effort :). I pushed my work to github for you to see.

I'm still not sure if the shop is intuitive yet though. To buy, sell, or trade, you need to press the confirm key on the item, then press the left or right arrows to choose how much you want to buy/sell/trade and press the confirm key to finish. I'm not sure but the number for how much you're buying/selling/trading might be hard to notice.

Some of the code is probably not needed anymore but I haven't removed any code yet. Should I work on removing this code or should we save it, just in case it ends up being needed?

So what's the next step? Refining the shop mode, removing code, fixing the trade system or something else?

@Bertram25
Owner

I finished fixing the bug somewhat. I got it working by returning to the root menu whenever a sale is made. I can change it so that it only goes to the root menu after everything is sold instead of after every single sale if you want, it will take a little more effort :). I pushed my work to github for you to see.
I'm still not sure if the shop is intuitive yet though. To buy, sell, or trade, you need to press the confirm key on the item, then press the left or right arrows to choose how much you want to buy/sell/trade and press the confirm key to finish. I'm not sure but the number for how much you're buying/selling/trading might be hard to notice.

I'll try it out and we'll see. :) thanks for the hard work done on the shop mode. You definitely didn't take the easiest thing to do and you're making progress anyway. So, my respect for that.

So what's the next step? Refining the shop mode, removing code, fixing the trade system or something else?

I'll try the code, but anyway the goal is to reach some usable state, better than it was before and look at the thing from that point.

@Bertram25
Owner

@codergreen :
I've just tried it. I didn't look at the code yet, but I like the general direction the buy/sell dialogues are taking.

I like the way you simply chose an item, set the number wanted and confirm. It's much more straight forward. :)

IMHO, here is what is needed to be done:

  • You can get rid of the validate option when it comes to buying selling normal stuff.
  • You should remove the item from the buy list when you bought every items available.
  • You should remove the buy/sell x columns since they're now useless.

I will also need to know how to test trading.

@codergreen

What do you mean by validate option? Do you mean how the menu displays more info about the item? If I remove the need for it for normal stuff, won't I need the buy/sell x columns? Or do you mean that pressing the confirm button on one of the normal items will purchase one? I just want to make sure I understand what you want.

I'll start working on the part where it removes the item from the buy list when you bought every items available. As for trading, it can't be done at the main menu unless you insert some code in a Lua file I think. I only have it working with the in-game shop at the moment. The GUI doesn't tell you what the shop keeper wants yet, but you get a healing potion if you give her a minor healing potion (don't worry this will be removed when I'm done with testing).

@Bertram25
Owner

What do you mean by validate option?

Ah, I meant the confirm tab next to the trade tab :)

won't I need the buy/sell x columns?

The 'buy/sell x' columns are the right-most ones when in buy or sell mode. They are already not working anymore anyway when I last tried your code, so I think you can remove them. I'm not speaking of the 'stock x' and 'owned x' one of course. :)

The GUI doesn't tell you what the shop keeper wants yet, but you get a healing potion if you give her a minor healing potion (don't worry this will be removed when I'm done with testing).

Np, feel free to do whatever is necessary for testing, I'll make corrective commits if needed.

Don't hesitate to keep asking questions if necessary. thanks a lot for the work done and a Happy New Year!!

@codergreen

I did what you requested :)
I removed the confirm tab and buy/sell x columns and made it so items are removed from buy list whenever you buy them all. The game crashes when you press the confirm key when you buy, sell, and trade everything, so I added a leave tab to quick fix the problem. If you don't want the leave tab, just ask; it should still be easy to fix.

I also played around with the trade system and code a little bit and RemoveObjectToSell might not be the problem, but I'm a little stuck on where the problem might be. I'll start using gdb on it to see where it says the problem is. I commited and pushed the code to github for you to review. Any suggestions are appreciated. And Happy New Year to you too :D

@Bertram25
Owner

I did what you requested :)

I've just seen the mail, thanks :) It's actually taking shape!! I'm sure we're close of the tidy up phase.

I removed the confirm tab and buy/sell x columns and made it so items are removed from buy list whenever you buy them all.

You left all the 'x 0' below the buy/sell column names you removed. Could you remove that as well?

The game crashes when you press the confirm key when you buy, sell, and trade everything, so I added a leave tab to quick fix the problem. If you don't want the leave tab, just ask; it should still be easy to fix.

I then shall ask for it :) But you can fix that last of all. Here is what I saw otherwise:
When you're at the root level or when seeing the buy/sell list of items, the textboxes saying: Purchases! +/-X , Sales: +/-X Total: X should be hidden.
They should be shown only when in the detailed description of the item you're in. I would even say only the 'purchases' should be shown when buying and only the 'sales' when selling.

You should also add a textbox, in the detailed item description screen, and under its description, saying:
"Push right to add items to buy/sell and left to remove items from your purchase/sale.", or something like that.

I also played around with the trade system and code a little bit and RemoveObjectToSell might not be the problem, but I'm a little stuck on where the problem might be. I'll start using gdb on it to see where it says the problem is. I commited and pushed the code to github for you to review. Any suggestions are appreciated. And Happy New Year to you too :D

Good luck and thanks :)

@codergreen

I haven't pushed anything to github yet, just came to say that I finished removing the "x 0" and made it so that purchases shows up when you buy and likewise for sales. After I fix a small bug, then I will work on the text box. As a note, when I removed the "x 0", it also removed "x number of items to buy/sell/trade" from the item display. It shouldn't be much trouble to add the "x number of items to buy/sell/trade" back in but considering that it probably won't be connected to anything, I might be able to place it somewhere else if you want (such as in the textbox that contains the text "Push right to add items to buy/sell and left to remove items from your purchase/sale.") Another question: Do you want the textbox to say "Push right to add items to buy/sell and left to remove items from your purchase/sale." for buy and sell or do you want it to say "Push right to add items to buy and left to remove items from your purchase." for buy and "Push right to add items to sell and left to remove items from your sale." for sell?

@Bertram25
Owner

Hi @codergreen ,
Cool to see improvements. The latter,if possible.
Thanks!

@codergreen

@Bertram25
Some new work commited and pushed. One bug though is that the sell menu doesn't show how many items are in stock, just how many you can sell. There was a similiar problem for the buy menu where you didn't know how much you had, only what was in stock, but I fixed it. Did I just introduce this problem or did this problem exist before? I'm leaning towards the first right now, but I was just wondering.

@rootslinux

I just checked the Allacrost code since I had it handy, and the sell menu does show how many items are in stock. It shows "Price, Stock, Own, Sell" data. And the buy menu similarly shows "Price, Stock, Own, Buy". I'm not sure what has changed so far between Allacrost and VT as far as the shop code goes though, so it may have been removed by Bertram or someone else before you started working on it. I kind of doubt it though, as that information is very useful to have in the menu.

@Bertram25
Owner

Well, to be completely honest, when you're selling something, how much is in stock of the shop isn't really important, so it's for the better, I'd say, just like that.
I will review your code now, and give maybe a better feed-back.

@Bertram25
Owner

It's starting to look actually fine!

I'd tweak a few things, but we're almost there:

Stuff I'd like to see before preparing a first merge:

  • The 'stock' column can be hidden/removed when selling. It isn't useful anyway.
  • The purchase/sale: +/- text should disappear when the number of sale/purchases is 0.

Could you do that ^ so we can prepare a first merge of your work together?

Stuff that should be done in priority afterwards:

  • It's cool you made the number of purchases/sales appear (xN in the text). Yet, we'll need to make it appear, in bigger, not mixed with the description/instructions text. (Why not next to the item name, or below, I don't know yet, what do you think?)

Stuff that we will have to do to put the finished touch:

  • Make sure everything is translatable
  • Come back to the the buy/sell list after buying/selling.
  • Remove all the dead code, once everything is known as fine.

Thanks a lot for the job done and best regards,

@codergreen

You reminded me of something. When there are no purchases or sales, how do I delete the text? Using an empty string with UTranslate produces alot of text and an empty string without UTranslate produces an error (SetOptionText needs UString).
To make something translatable I should use UTranslate right?
Another task that I'll need to is comment the code I've been making and possibly change comments on some of the code. I know it's bad practice to not comment code soon after making it but I procrastinate and make excuses. So I'll work on that if everything works out with the above tasks.

@Bertram25
Owner

Hi @codergreen :)

You reminded me of something. When there are no purchases or sales, how do I delete the text? Using an empty string with UTranslate produces alot of text and an empty string without UTranslate produces an error (SetOptionText needs UString).

You can simply call TextBox::ClearText() in that case, IMHO.

To make something translatable I should use UTranslate right?

Translate for normal strings, and UTranslate for ustrings, indeed.

Another task that I'll need to is comment the code I've been making and possibly change comments on some of the code. I know it's bad practice to not comment code soon after making it but I procrastinate and make excuses. So I'll work on that if everything works out with the above tasks.

You're the boss. I'll make a more thorough technical review once we've reached a step where the merge is ready to processed.

Thanks and best regards,

@codergreen

I did most of what you said: stock column is hidden in sell tab, no purchase/sale appears when balance is 0, I think everything is translatable, and when you buy or sell it doesn't return to root shop menu.
Also apparently the game still crashes when there are no available selection, such as buy, sell, or trade. Rather than fix the option box, I made it so the leave tab appear when there are no other available options. If you definitely don't want the leave tab or don't want to leave an option box error, I could look into fixing it.
Another thing I haven't fixed yet is the placement of the buy/sell/trade amount. I can place it anywhere but where it used to be because other text is there (I think the columns auto space themselves apart). I can keep looking for a way to add it to info mode and not list mode, but it might be challenging.
Finally, I haven't removed unused code yet, just in case.

@Bertram25
Owner

Hi @codergreen

Also apparently the game still crashes when there are no available selection, such as buy, sell, or trade. Rather than fix the option box, I made it so the leave tab appear when there are no other available options. If you definitely don't want the leave tab or don't want to leave an option box error, I could look into fixing it.

No, don't bother, sounds perfect to me :)

Another thing I haven't fixed yet is the placement of the buy/sell/trade amount. I can place it anywhere but where it used to be because other text is there (I think the columns auto space themselves apart). I can keep looking for a way to add it to info mode and not list mode, but it might be challenging.

If only that is left, then, we'll have a look at it once the code has been merged back to master. So, I'd say, don't worry to much about this for now.

Also, I wondered in which state you left the trade feature? Do you want me to test it as well, or is it not ready yet?

Finally, I haven't removed unused code yet, just in case.

Sounds fair. I'll review the code and the feature asap and give a proper feed-back on it. I'm also inviting everyone else to do a code review, if they can. The more eyes on it, the better. ( ping @IkarusDowned )

Thanks a lot for that marvellous work done so far!

@codergreen

The trade feature works somewhat and has been updated with the buy and sell tabs, but there are still a few things I need to add, such as fixing the crashing problem when all of an item is traded for, showing what you're trading, and adding money costs to trades too. But it could receive some testing if you want, it should be ready for that :)
Something I forgot to mention was I didn't comment the code yet. Should I do that now?

@IkarusDowned

if you feel like your code is at a state that you would like us to code-review, what i would suggest is commiting onto your branch (whichever that is) and then submitting a pull request. I'd be happy to go over it as well :)

@Bertram25
Owner

@codergreen Hi :)

I've made a bit of work on polishing the shop mode, thanks to your core work:
c8cf779

It should be a bit more easy to finish working on the trade mode now.

Best regards,

@codergreen

Thank you for the help. I've been busy with other things but I'll try to get some work done.I'll add comment to the code if you haven't done that already. After that I'll work on the trade mode if I haven't left anything undone.

@Bertram25
Owner

Np :-) I just wanted to polish things slightly and give you some room for what's next.

@Bertram25
Owner

Hi again, @codergreen :)

I've locally done a few more fixes with the actual hope to bring the new shop and the world map and improved quest log in for RC3. There won't be trading for now. Yet, I've locally split the standard price and the trade price loading, and here is a proposal to display the trading conditions, taken from almost actual debug shop:

trade_mock_up

Once I've pushed my small changes, what's left to achieve that is:

  • Moving the character party to the left when in the trade info mode, and displaying equipment.
  • List in an option box the trading conditions (with possible scrolling of the conditions when there are many using up and down keys).

What do you think about it?

Best regards,

@Bertram25
Owner

Ah also, according to what I saw:

such as fixing the crashing problem when all of an item is traded for

This bugs happens because the ShopMode::_available_sell list isn't updated, making the sell interface crash when trying to find the global object behind the ShopObject that doesn't exist anymore.

In ShopMode::CompleteTransaction(), I simply moved those calls :
_UpdateAvailableObjectsToSell();
_UpdateAvailableShopOptions();

before
_sell_interface->TransactionNotification();

and the crash is gone. :)

I'll commit something like tonight I guess.

@codergreen

Wow the interface looks great @Bertram25 and thank you for finding the fix to the bug :) What do you need me to do right now?

@Bertram25
Owner

Hi @codergreen :)

I've just pushed the latest things to the shop making the trade mode working so far, as for me.

What's left is this:

  • Moving the character party to the left when in the trade info mode, and displaying equipment.
  • List in an option box the trading conditions (with possible scrolling of the conditions when there are many using up and down keys).

See the mockup. Are you interested?

@codergreen

@Bertram25
Sure I'll do it if you don't need it done too soon. My weekdays have been busy recently, but I'll gladly help :)

@Bertram25
Owner

@codergreen
Np, take your time. :)

@codergreen

@Bertram25
Thanks for the time, I got some work done :) . The characters were moved to the left for trade menu and displaying trade conditions including quantity are working with scrolling. A few things still need added though. One of the problems that needs fixed is about resizing the icons for the items in condition list. The second problem is that I could find room for quantity numbers (although looking at the picture above, I could move all trade condition information over). Finally, I'd have to add arrows that display when there is more trade condition than the player can see. What code should I use to display them?

@Bertram25
Owner

Hi @codergreen,

Cool!

One of the problems that needs fixed is about resizing the icons for the items in condition list.

Hmm, IIRC, for an option box, you can set an embedded image to it (I guess that's waht you did),
and then do somethign like that:

_option_box_object.GetEmbeddedImage(index)->SetHeightKeepRatio(desired_height);

Finally, I'd have to add arrows that display when there is more trade condition than the player can see. What code should I use to display them?

The option box object automatically draws arrows if the number of options it has got is higher that what it can draw. (See OptionBox::Draw())
You'll simply have to setup a correct dimension rectangle for the option box object and it's done (hopefully). :)

You'll also have to catch up/down key input to scroll it when possible.

Thanks a lot for the work done!

@IkarusDowned

Also, make sure you enable scrolling in the option box. Take a look at the Item menu Option Boxes for examples

@codergreen

@Bertram25
I'm having some trouble getting the game to not crash whenever I select an item to trade for that has more than one item requirement. I haven't bothered with adding the code for input yet because of that. I read the comment above by IkarusDowned, so I will look into how to enable scrolling in case it hasn't been done yet. Thanks again for all your patience. :)

@Bertram25
Owner

@codergreen Hi :)

I'm having some trouble getting the game to not crash whenever I select an item to trade for that has more than one item requirement. I haven't bothered with adding the code for input yet because of that.

I can take care of the crash if you want me to. If I get it right, it happens with the Paladin's sword in the debug shop, right?

Thanks again for all your patience. :)

Don't worry about it, have fun!

@codergreen

You got it right, the Paladin's sword in the debug shop.

@Bertram25
Owner

Ok, can I first import your two patches on master?

@codergreen

I just merged the branches trade and master. Did that work?

@Bertram25
Owner

Sorry, I can't tell. According to what I see, I think not. Yet, it seems you pushed another commit. Is that what you meant?

@codergreen

I checked my repository and it looks like I copied the commits from the trade branch onto the master branch. Is that what you wanted?

@Bertram25
Owner

Ahh that.

I don't needed that, don't worry. I'll import your commits as soon as I've figured out the bug and fixed it.

But thanks anyway. :)

@Bertram25
Owner

Hi :)

I've found the bug and locally finished the feature.

In fact the conditions option box embedded image SetDimension() call was faulty since the index given was wrong, as your option box has got two columns, so if you wanted to get the embedded image you had to manage another integer index and increment it by two. There also was a memleak you didn't destroyed the new GlobalObject* created.

As the conditions option box had map layout problem (You set a width of 600 to it to try and fix the xN text placement), I split the option boxy into two to get the correct layout (and a fixed one).

I'll merge your two commits into master and add my modifications asap.

Once this is all in, I'll have to test whether complex trades (like the paladin's sword) are working right.

Thanks a lot for the work of the feature. :)

@Bertram25
Owner

I pushed your work as:
6820cc2

And basically finished the feature with:
83fc258

I think this is all done, and need harder testing to discover other needs or flaws. Thanks a LOT for the work done on it :) I'm closing this issue. Let's discuss potential other needs in another one.

@Bertram25 Bertram25 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.