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

Sign calculator #686

Merged
merged 20 commits into from
May 14, 2024
Merged

Sign calculator #686

merged 20 commits into from
May 14, 2024

Conversation

olim88
Copy link
Contributor

@olim88 olim88 commented May 5, 2024

adds ability to do calculations when inputting number into signs and a calculate command

sign input

image
image

command

image
image

I have created custom calculator utility for this using a shunting yard algorithm to convert calculation to RPN and then calculating the answer. it also supports (s, k, m, b) and created tests for this.

also tested in game bazaar and ah

@viciscat
Copy link
Collaborator

viciscat commented May 5, 2024

Another banger by olim. Do you think it would be doable to have an interface that a screen could implement to have it somewhere else? i.e. the popup to set the price/bid in fancy AH.

@olim88
Copy link
Contributor Author

olim88 commented May 5, 2024

implement to have it somewhere else

This should not be to hard. If the renderSign method in SignCalculator to a location (add possible renamed) then it should work were ever this is called. is this the sort of thing you meant

@Fluboxer
Copy link
Contributor

Fluboxer commented May 5, 2024

add "e" for "enchanted", equals 160 (2.5 stacks)

@viciscat
Copy link
Collaborator

viciscat commented May 5, 2024

This should not be to hard. If the renderSign method in SignCalculator to a location (add possible renamed) then it should work were ever this is called. is this the sort of thing you meant

yea that's good enough

@kevinthegreat1
Copy link
Collaborator

Maybe only works if you have a = sign in front like =4s * 5. Also would be nice to have purse be the number of coins you have in your purse.

@kevinthegreat1 kevinthegreat1 added new feature This issue or PR is a new feature reviews needed This PR needs reviews labels May 5, 2024
@olim88
Copy link
Contributor Author

olim88 commented May 6, 2024

add "e" for "enchanted"
have purse be the number of coins you have in your purse

good idea. i have added this and put a key in the tooltip so players know its there

Maybe only works if you have a = sign in front

i feel like this is a nice idea and some will like it however i like to be reassured what the value of the sign is going to be even if i am just doing something like 1.02k so i have made it a toggle in the settings so both options are available

@viciscat
Copy link
Collaborator

viciscat commented May 7, 2024

last nitpick and I think this PR will be good, add "p" for "purse" maybe? to make it easier and faster to type

@Fluboxer
Copy link
Contributor

Fluboxer commented May 7, 2024

last nitpick and I think this PR will be good, add "p" for "purse" maybe? to make it easier and faster to type

I still don't get how this one would be useful tho

@viciscat
Copy link
Collaborator

viciscat commented May 8, 2024

I still don't get how this one would be useful tho

make it faster to type or something, faster to type "p" than "purse"

@LifeIsAParadox LifeIsAParadox added merge conflicts This PR has merge conflicts that need solving. reviews needed This PR needs reviews and removed reviews needed This PR needs reviews merge conflicts This PR has merge conflicts that need solving. labels May 9, 2024
@kevinthegreat1
Copy link
Collaborator

Can you do git pull master and git rebase master instead of merging master.

@kevinthegreat1 kevinthegreat1 added changes requested This PR need changes and removed reviews needed This PR needs reviews labels May 9, 2024
olim88 added 5 commits May 9, 2024 22:42
added magnitude e as valid. and added to EditBidPopup.
also add key for magnitudes into option tooltip
also fix code a tad
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels May 9, 2024
src/main/resources/assets/skyblocker/lang/en_us.json Outdated Show resolved Hide resolved
@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels May 10, 2024
make text for pr in correct order in file
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels May 10, 2024
@kevinthegreat1 kevinthegreat1 added the bleeding edge This PR has been accepted into bleeding edge label May 11, 2024
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a tiny bug when you have requiresEquals set to true and type a valid equation without the equals sign. When you add the equals sign, it will show as invalid since the equation is not reevaluated when an equals sign is added, but I don't see a good way of fixing this.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels May 12, 2024
@kevinthegreat1 kevinthegreat1 dismissed stale reviews from viciscat and AzureAaron May 12, 2024 02:31

comment / trivial

@AzureAaron AzureAaron merged commit 992ee43 into SkyblockerMod:master May 14, 2024
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label May 14, 2024
@kevinthegreat1 kevinthegreat1 removed the bleeding edge This PR has been accepted into bleeding edge label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants