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

Potential fix for issue #107 #118

Open
wants to merge 3 commits into
base: plank-sack
Choose a base branch
from

Conversation

YeahImRose
Copy link

Hello!
This is a potential fix for issue #107. I have not yet done rigorous testing, and lack the ability to test if my Plank Make fix works due to lack of the magic level to cast it. I'm fairly sure there's gonna be some other context menu that uses "Make" for its menu option name, which will need a check for.

Feel free to suggest any changes to the actual formatting of the code as well, I just whipped this up real quick.

Thanks!

@YeahImRose YeahImRose marked this pull request as ready for review July 7, 2024 04:56
@YeahImRose
Copy link
Author

This now only needs review on if it catches Make Plank correctly. @Enriath

@Enriath
Copy link
Owner

Enriath commented Jul 13, 2024

So, this isn't something I'm able to test, as I do not have an active sub. I also don't have the money to buy a sub for such a short amount of time, both in game and IRL.

But even still, I feel like this is quite overengineered. Unless there's some super sneakret training method I cannot even begin to fathom, there's not going to be a situation where logs will become planks & planks will get used in a way that causes issues, so having an entirely separate snapshot (and all the infrastructure to support it) feels very extra.
Putting them together would require a bit of a rewrite to the deltaPlus and deltaMinus parts, but it would cut down on the amount of code heavily. Merging together the log and plank ID lists into a HashSet would also likely help; The plank lists were only lists to make associating the names and IDs together easier, though I likely could have written that better with an enum had I thought ahead.

@Enriath Enriath added enhancement New feature or request plank-sack bug Something isn't working and removed enhancement New feature or request labels Jul 13, 2024
@YeahImRose
Copy link
Author

So, this isn't something I'm able to test, as I do not have an active sub. I also don't have the money to buy a sub for such a short amount of time, both in game and IRL.

But even still, I feel like this is quite overengineered. Unless there's some super sneakret training method I cannot even begin to fathom, there's not going to be a situation where logs will become planks & planks will get used in a way that causes issues, so having an entirely separate snapshot (and all the infrastructure to support it) feels very extra. Putting them together would require a bit of a rewrite to the deltaPlus and deltaMinus parts, but it would cut down on the amount of code heavily. Merging together the log and plank ID lists into a HashSet would also likely help; The plank lists were only lists to make associating the names and IDs together easier, though I likely could have written that better with an enum had I thought ahead.

I actually made this because UIMs have several methods that include going to chop trees and make planks from those logs. I definitely agree that the solution I wrote was overkill, but I didn't want to refactor the entire plugin haha

@YeahImRose
Copy link
Author

I should note that I'm down to do a big cleanup, just a matter of never having heard back from you yet so I didn't know if it'd be worth the effort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working plank-sack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants