tesu / PennyPincher

XIVLauncher plugin for undercutters
MIT License
16 stars 21 forks source link

Issue with the modulo operation #28

Closed Lochrine closed 2 years ago

Lochrine commented 2 years ago

Hello, first I wanted to say thanks for creating this. It's a very useful plugin.

I wanted to leave a comment because I found that the modulo config setting is currently not working as described. When setting the modulo amount, the modulo is subtracted first and then the price to undercut is subtracted. So for example, if I wanted to configure my settings in such a way as to always undercut by 10 gil instead of 1, this is actually not possible under the current settings. I believe I have found the relevant line here, line 210 in PennyPincher.cs:

var price = listing.ItemListings[i].PricePerUnit - (listing.ItemListings[i].PricePerUnit % configuration.mod) - configuration.delta;

Which I believe should be changed to something like this:

var price = listing.ItemListings[i].PricePerUnit - configuration.delta;
price -= (price % configuration.mod);

So for example, a listing of 867 with configuration settings of 1 gil delta and 10 gil modulo would go like: 867 - 1 = 866 => 866 - (866 % 10) = 860.

tesu commented 2 years ago

I agree that you can't undercut by at least 1 and also have your copied price end in 0 or 00 using a modulo of 10 or 100. You can use delta = 0 to ensure a price that ends in 0 or 00, but that doesn't guarantee that you're undercutting by >0.

However, your suggestion would make it impossible to have prices that always end in other (non-zero) digits, e.g. XXX99 or XXXX9. You can currently do this by first zeroing out the last few digits using mod 10/100/1000/etc, and then subtracting out whatever delta necessary to get the end digits you want. It would be impossible if the modulo was applied last since you would always end up with a price divisible by modulo (and numbers ending in 99 don't share a divisor).

I think this was the original motivation for #9 (I don't use this feature myself).

Feel free to create a PR to add a second modulo term if you want.

tesu commented 2 years ago

In any case, this is a duplicate of #20.

Lochrine commented 2 years ago

I see. So that is actually by design from a request. Interesting. Fair enough, I'll submit a PR whenever I have some free time. Thanks!

Lochrine commented 2 years ago

Hello, I created the changes necessary for this functionality, but every time I try and push a new branch to the repository, I am getting a 403. It is a public repository, so I'm not sure why this is occurring. I tried all three different login options that I had through Visual Studio. Is there anything else I need to do, or anything you recommend to resolve this?

Lochrine commented 2 years ago

Nevermind, you can disregard the last comment. I was able to fork the repository and then create a PR from there. Apologies, this is my first submission to a public project. Thanks!