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

Aerial Fishing Pearl Luck Update to Version 1.2 #6958

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

H4waiianPunch
Copy link
Contributor

Looking to Merge in the changes I've made to my plugin.

  • More stats
  • More control for users
  • Hopefully I did everything right this time..!

@runelite-github-app
Copy link

runelite-github-app bot commented Nov 11, 2024

Copy link
Member

@tylerwgrass tylerwgrass left a comment

Choose a reason for hiding this comment

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

  • Saving multiple config options every fish attempt seems a bit much. I would consider saving maybe once a user ends a session instead? (leaves the island, logs off, shuts down plugin, etc)
  • It doesn't really make sense to use the config checkboxes to reset stats
  • You are calling pearlRateWikiCalc even when the user is doing fishing/hunter activities that aren't aerial fishing
  • Less of a blocker maybe, but in my opinion I don't think it's necessary to provide a config option to show/hide every field in the overlay.

@H4waiianPunch
Copy link
Contributor Author

  • Saving multiple config options every fish attempt seems a bit much. I would consider saving maybe once a user ends a session instead? (leaves the island, logs off, shuts down plugin, etc)

Ah, that's a very valid point. Didn't even think about it making constant calls. That's fixed.

* It doesn't really make sense to use the config checkboxes to reset stats

I wasn't sure how else to do this as I couldn't find a way to just put a new "Reset" button in or anything else. What do you suggest for this?

* You are calling `pearlRateWikiCalc` even when the user is doing fishing/hunter activities that aren't aerial fishing

This will take some more looking into, but I'll fix this.

* Less of a blocker maybe, but in my opinion I don't think it's necessary to provide a config option to show/hide every field in the overlay.

I had a user that wished it to be fully customizable so they could choose what they see, I think it's a good option for people these days. Everyone always asks for a toggle after all!

@tylerwgrass
Copy link
Member

I wasn't sure how else to do this as I couldn't find a way to just put a new "Reset" button in or anything else. What do you suggest for this?

You can add menu entries to the overlay, which will appear when you shift + right click.
image
example

  • You are calling pearlRateWikiCalc even when the user is doing fishing/hunter activities that aren't aerial fishing

This will take some more looking into, but I'll fix this.

You could probably add a regionId check for the island

@H4waiianPunch
Copy link
Contributor Author

Hi Tyler,
I think this new version should meet your requirements now.

I decided to not go with the region ID option(as I couldn't quickly figure it out). Instead, I made sure that the hunter and fishing calcs done in pearlRateWikiCalc were only being done if the overlay was added.

Let me know if there's anything else!

Copy link
Member

@tylerwgrass tylerwgrass left a comment

Choose a reason for hiding this comment

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

If you would really like to keep the fully customizable panel, then I would recommend storing the config values for use outside of the overlay render method as constantly fetching them from the config in render could have performance impacts.

Less importantly it looks like src/main/java/com/AerialFishingPearlLuck/PearlCalcType.java is unused

@H4waiianPunch
Copy link
Contributor Author

First off, thank you for the approval.

If you would really like to keep the fully customizable panel, then I would recommend storing the config values for use outside of the overlay render method as constantly fetching them from the config in render could have performance impacts.

I'll have to try and figure out how this works, I don't want to cause performance issues for users.

Less importantly it looks like src/main/java/com/AerialFishingPearlLuck/PearlCalcType.java is unused

You're right, I was using that and then changed how things worked. Thank you for bringing it up, I'll be sure to remove it.

@LlemonDuck
Copy link
Contributor

I'll have to try and figure out how this works, I don't want to cause performance issues for users.

in onStartUp, and onConfigChanged, read the config entries and store them in variables. Then use those variables during the render call

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin change waiting for author waiting for the pr author to make changes or respond to questions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants