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

Make TOA killcount PB report challenge time #616

Open
2 tasks done
BlackCoyote opened this issue Nov 27, 2024 · 4 comments
Open
2 tasks done

Make TOA killcount PB report challenge time #616

BlackCoyote opened this issue Nov 27, 2024 · 4 comments
Labels
enhancement Improvement to an already existing notifier Notifier: Kill Count

Comments

@BlackCoyote
Copy link
Contributor

BlackCoyote commented Nov 27, 2024

Checklist

  • I've searched the issues and pull requests for similar looking suggestions.
  • I've checked the Unreleased section of the changelog for newly added features that sound like my suggestion.

Describe your Suggestion

Curently, the killcount notifier includes the total completion time when completing a Tombs of Amascut raid. Since an exception was made for the Theatre of Blood to explicitly look for the challenge time, I think it would be nice to also do the same for Tombs of Amascut.

The TOA completion message when a new challenge time PB is achieved:
toa

the extra metadata of the notifier message that was sent:

  "extra": {
    "boss": "Tombs of Amascut: Expert Mode",
    "count": 569,
    "gameMessage": "Your completed Tombs of Amascut: Expert Mode count is: 569.",
    "time": "PT29M40S",
    "isPersonalBest": false,
    "party": [
      "The Bc"
    ]
  },
@BlackCoyote BlackCoyote added the enhancement Improvement to an already existing notifier label Nov 27, 2024
@iProdigy
Copy link
Member

aren't the toa CAs based on total time, rather than challenge time?
tob was adjusted as an exceptional case bc those CAs use challenge time

@BlackCoyote
Copy link
Contributor Author

BlackCoyote commented Nov 27, 2024

I'm not sure which times are required for CA's in neither TOA nor TOB, the wiki doesn't really seem to specify at a glance.

I didn't really think about it in the context of Combat Achievements, but instead of in the context of what RuneLite considers your personal best time. When you use the !pb chat command for TOA, it reports the challenge time, just like for TOB, so I expected the same time to be relevant for this notifier, but I can also see the merit in keeping it consistent in what the speed Combat Achievements for that raid are based on.

If you think it's preferred to keep the overal time for TOA PBs, it would be nice though if the challenge time and whether it is a challenge time PB could also be included in the metadata under separate names, so that custom endpoints can choose whether they want to keep it consistent with RuneLite's chat command PB, or with the relevant Combat Achievement criteria. Something like this:

    "time": "PT29M40S",
    "isPersonalBest": false,
    "challengeTime": "PT26M17S",
    "isChallengePersonalBest": true,

@iProdigy iProdigy added the needs discussion Discussion is required on whether the requested feature should be added label Nov 27, 2024
@iProdigy
Copy link
Member

Based on the CA argument, we will keep the TOA PB as the total time for now, but we plan to report both the total time and challenge time in the notification metadata. Additionally we will consider reporting whichever time is a new personal best in the notification text, with options for user customization

if you want to take a shot at this approach, feel free!

@iProdigy iProdigy added Notifier: Kill Count and removed needs discussion Discussion is required on whether the requested feature should be added labels Nov 28, 2024
@BlackCoyote
Copy link
Contributor Author

Having both times and PB bools included in the metadata would be great! (both times could be a PB at the same time)

I unfortunately don't feel confident at all that I would be able to figure this out and implement it in a satisfactory manner, so I'm hoping you might find some time for it instead.

I'd be happy to build it locally and run some raids to test though, if it's any help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an already existing notifier Notifier: Kill Count
Projects
None yet
Development

No branches or pull requests

2 participants