Skip to content

Conversation

@mathclassjr
Copy link

Hey Matsyir,

This is my first contribution to open source development. I'm not super familiar with Java but decided to try and take a stab at leveraging the player's stats when performing damage calculations along with displaying that information on the fight detail log. I also attempted to implement a hiscore lookup for an opponent in normal fights but the Hiscore Manager class doesn't always return back data so if it doesn't return data I defaulted it to the configurable stats. I wasn't sure how best to implement this nor how best to make it a configurable option so I know this pull request will need some work but will appreciate any feedback.

Thank you for this plugin. I use it all the time and it's definitely helped up my PvP game as I'm sure countless others as well.

@mathclassjr mathclassjr changed the title feat/Tracker to use player's current stats or highscore look for normal fight types feat/Tracker to use player's current stats or highscore lookup for normal fight types Aug 6, 2025
@mathclassjr mathclassjr changed the title feat/Tracker to use player's current stats or highscore lookup for normal fight types feat/Tracker to use player's current stats or hiscore lookup for normal fight types Aug 6, 2025
@Matsyir
Copy link
Owner

Matsyir commented Sep 6, 2025

I merged other PRs, which caused merge conflicts in yours - fixed those for you.

Before reviewing & merging, I would appreciate if you adjusted the styling of your code to closer match the codebase's. Notably, there's a lot of whitespace-only changes where you add tabs everywhere without even changing nearby code, e.g:
https://github.com/Matsyir/pvp-performance-tracker/pull/65/files?w=0#diff-f0d6f23e5cf9efc6af7e8b03659e0e0afec4d8f0b15dc341fa9bdb993f49a7caR326

Also, we want to avoid import package.module.*, list out the specific imports required and remove unused ones.

The simplest way to do this (you shouldn't have to do much manual tweaking for this stuff) is probably to just import RuneLite's style configuration into IntelliJ, assuming that's what you're using: https://github.com/runelite/runelite/wiki/Code-Conventions

From there, I'm not sure if it applies automatically or if you have to go re-save all your changed files. I'm generally not too much of a stickler for the code style, but I can definitely appreciate when everything is as consistent as possible, and these extra unnecessary whitespace changes just kinda pollute the diff for no reason - rather address it now than accommodate it and have it remain an issue for any future PRs.

Screenshot 2025-09-06 112203

Please do not build pyramids in the code
(I'm just teasing, it's not that serious haha)


My thoughts regarding the feature (but fwiw, I haven't gone through the code much yet)

I always preferred to avoid using the hiscores so the plugin can work consistently and not swamp the hiscore servers (they were frequently having server issues during early stages of the plugin, when it was initially considered)... I dunno, I don't really like the whole "it's gonna use hiscores, but it might also fail and just use config anyways" vibe. At the same time, we are already using hiscores at this point I believe, for HP, for the ko chance statistic... so it's like, why not.

Perhaps we could just leave an option to opt-out of using hiscores (well, I'd make it an opt-in, but on by default).

Also, maybe you already did it to some extent, but alongside this, we should ensure that we are retrieving and/or initializing levels the same way everywhere, so we don't have to duplicate any logic regarding detection for lms, pvp arena etc. I feel like that logic is likely a bit all over the place at this point (but again, didn't look at your PR too in depth yet)

Also, if not already done this way, we should probably ensure that we only call the hiscore once when the fight starts, and not potentially multiple times in a fight. Although I believe RL does cache the hiscore results anyways, it would be more efficient to just get it once and save it I'd think.

Lastly, my apologies for the delay in getting around to this. Busy summer. Thanks for the kind words regarding the plugin! Same here. The plugin started out a private project for my own improvement, since it was before the plugin hub existed and the RL team wasn't willing to have it as an official plugin. It's crazy how far it's come since then!

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.

2 participants