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

Add feature for automatic calculation of barter money #311

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

y0lo
Copy link
Contributor

@y0lo y0lo commented Jun 18, 2023

Calculate barter change money automatically.
It's still possible to chose quantity of money manually.

Demo:
https://m.youtube.com/watch?v=jK6BukxqeW0

Rationalization:

  1. It's hard to play on devices without keyboard. So it's not possible to enter number of items by using keyboard.
  2. There is an issue on android devices the fast inventory number scroll does not work.
    Even the mouse button is pressed and not released the loop is ended:
    https://github.com/alexbatalov/fallout2-ce/blob/main/src/inventory.cc#L5629
    the mouse event is reset.

These points make it difficult to trade with NPCs on android devices.

@y0lo
Copy link
Contributor Author

y0lo commented Jun 18, 2023

@alexbatalov please, have a look.

src/inventory.cc Outdated Show resolved Hide resolved
src/inventory.cc Outdated Show resolved Hide resolved
@y0lo y0lo force-pushed the feature/y0lo/fast-barter-auto-calc-change-money branch from 0d50964 to 0e806d9 Compare June 20, 2023 09:21
@y0lo y0lo requested a review from phobos2077 June 20, 2023 11:55
@phobos2077
Copy link
Contributor

The code looks good, but I have some reservation about the feature. What if I (for whatever reason) want to gift money to the trader? Can I add more money this way? Usually with sfall features like this are either optional or implemented via some mod. Although in this case you can argue it's essential QoL improvement. But often there are some use cases that features like this might break, so you need to think it through.

I think more "vanilla-friendly" way would be to show the window like before, but pre-fill it with the "recommended" amount of money.

@Lexx2k
Copy link

Lexx2k commented Jun 20, 2023

imo the popup for the amount should always show up. Could be pre-filled with a break-even amount in case of bottle caps, I guess, but that's as far as I would go. Also would be good to have an option somewhere to define more protos as money-type, in case mods are adding multiple currencies.

@phobos2077
Copy link
Contributor

In future we could move this to script-based mod. I agree about multiple currencies. Even though the engine is filled with if SOME_ID == WHATEVER I think for a "community" engine we should strive for universal solutions.

@y0lo
Copy link
Contributor Author

y0lo commented Jun 20, 2023

It's still possible to select quantity manually eitehr move mony second time (first move - added needed amount, second move - open popup) or move money as first item.
It's working, please test by yourself.

@y0lo
Copy link
Contributor Author

y0lo commented Jun 20, 2023

imo the popup for the amount should always show up. Could be pre-filled with a break-even amount in case of bottle caps, I guess, but that's as far as I would go. Also would be good to have an option somewhere to define more protos as money-type, in case mods are adding multiple currencies.

The caps/money id is hardcoded and it's used to display money in small left screen in dialogs and check money in scripts. If someone wants to use several currencies it will be breaking changes.

@Lexx2k
Copy link

Lexx2k commented Jun 20, 2023

imo the popup for the amount should always show up. Could be pre-filled with a break-even amount in case of bottle caps, I guess, but that's as far as I would go. Also would be good to have an option somewhere to define more protos as money-type, in case mods are adding multiple currencies.

The caps/money id is hardcoded and it's used to display money in small left screen in dialogs and check money in scripts. If someone wants to use several currencies it will be breaking changes.

We already have mods for that in Fo2, it's not impossible to do.

I think moving money twice to get the popup is an unnecessary inconvenience.

@phobos2077
Copy link
Contributor

phobos2077 commented Jun 20, 2023

I think to show popup with a pre-filled amount should work for everyone, is it not? We can always replace it later with some mod (when we have proper tools for that), but for now seems like a good compromise.

@y0lo
Copy link
Contributor Author

y0lo commented Jun 20, 2023

I think to show popup with a pre-filled amount is a good option that should work for everyone, is it not?

Yes, the pre-filled quantity is possible. I can add this.
In my point of view this approach is less confusing but less convenient.

@Lexx2k in which modes there are used several currencies? I'm playing in Nevada there are no caps but the money item have valid MONEY id.

@Lexx2k
Copy link

Lexx2k commented Jun 20, 2023

Personal mod that hasn't been released (yet). It's old, though, and I'm not sure when it will be released.

The feature works in any direction to move from/to player/npc and from/to inventory/table
@y0lo
Copy link
Contributor Author

y0lo commented Jun 21, 2023

@phobos2077 @Lexx2k
Fixed: set recommended amount of money to be moved for barter in the selection window.
Now it works in any direction from/to player/npc from/to table/inventory.

src/inventory.cc Outdated Show resolved Hide resolved
@y0lo
Copy link
Contributor Author

y0lo commented Jul 19, 2023

Added demo video with the usage of the feature:
https://m.youtube.com/watch?v=jK6BukxqeW0

@phobos2077
Copy link
Contributor

phobos2077 commented Jul 19, 2023

I like it, looks "vanilla" enough (as you still need to drag money around instead of complete money automation) but alleviates the most annoying part of calculating numbers. Now I want this in sfall! :D

Should probably be possible to implement via script. I think Stalin's Inventory Filter solves this problem in somewhat different way. Can use similar approach to match the behavior of this PR. Or not, if we can't manipulate the item count popup window via script.

@phobos2077
Copy link
Contributor

I implemented this into sfall. And I think this feature should be optional. Maybe some people like to do these math calculations in their head. There's a new INI setting in related PR, but better wait until it's merged to have the same INI keys if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants