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

Add support for Skill Challenge Clues #9000

Merged
merged 4 commits into from Aug 9, 2019

Conversation

@Hydrox6
Copy link
Contributor

commented Jun 2, 2019

Implements part of #8504, Fixes #2053

Works with both Charlie Challenges and Sherlock Challenges.

Since Item Requirements aren't only an emote thing, I've moved them to their own package, and moved the shorthand constructors to their own file. I've also added a simple xOfItem requirement, for the multiple challenges that need a certain amount of something.

All challenges were gotten from the wiki, though there are 2 challenges I'm not entirely sure about:

  • Slay a Nechryael doesn't have a full stop, but every other challenge does. I'm not sure if Jagex forgot it.
  • Cook a swordfish. specifies that the player needs to catch the swordfish on the wiki

Some of the challenge text might be completely wrong, however, as I found with Enchant a piece of dragonstone jewellery. which is listed on the wiki as Cast Lvl-5 Enchant

@Hydrox6 Hydrox6 changed the title Clue challenge Add support for Skill Challenge Clues Jun 2, 2019

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch from 7fefdfc to a85f13b Jun 2, 2019

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

0b813f1 doesn't compile. ItemRequirements::xOfItem() seems to have been added to this commit instead of to a85f13b in error. (maybe xOfItem and MultipleOfItemRequirement should be in a commit between that one and "Add support for Challenge Clues"?)

@JacobThompson
Copy link
Contributor

left a comment

Looks good keep up the good work!

Show resolved Hide resolved ...va/net/runelite/client/plugins/cluescrolls/clues/SkillChallengeClue.java Outdated
Show resolved Hide resolved ...va/net/runelite/client/plugins/cluescrolls/clues/SkillChallengeClue.java Outdated
any("Earth Rune x15", xOfItem(ItemID.EARTH_RUNE, 15), xOfItem(ItemID.DUST_RUNE, 15), xOfItem(ItemID.MUD_RUNE, 15), xOfItem(ItemID.LAVA_RUNE, 15), item(ItemID.STAFF_OF_EARTH), item(ItemID.EARTH_BATTLESTAFF), item(ItemID.MYSTIC_EARTH_STAFF), item(ItemID.MUD_BATTLESTAFF), item(ItemID.MYSTIC_MUD_STAFF), item(ItemID.DUST_BATTLESTAFF), item(ItemID.MYSTIC_DUST_STAFF), item(ItemID.LAVA_BATTLESTAFF), item(ItemID.MYSTIC_LAVA_STAFF), item(ItemID.LAVA_BATTLESTAFF_21198), item(ItemID.MYSTIC_LAVA_STAFF_21200)),
any("Unenchanted Dragonstone Jewellery", item(ItemID.DRAGONSTONE_RING), item(ItemID.DRAGON_NECKLACE), item(ItemID.DRAGON_BRACELET), item(ItemID.DRAGONSTONE_AMULET))),
new SkillChallengeClue("Craft a nature rune.", item(ItemID.PURE_ESSENCE)),
new SkillChallengeClue("Catch a mottled eel with aerial fishing in Lake Molch.", any("Fish chunks or King worms", item(ItemID.FISH_CHUNKS), item(ItemID.KING_WORM)), emptySlot("No Gloves", EquipmentInventorySlot.GLOVES)),

This comment has been minimized.

Copy link
@Nightfirecat

Nightfirecat Jun 6, 2019

Contributor

I'm pretty sure the item requirements for this clue are not correct. I think you do need to have no gloves equipped, but you also need to have Cormorant's glove equipped. That should be listed instead of (or in addition to?) the no-gloves requirement.

This comment has been minimized.

Copy link
@Hydrox6

Hydrox6 Jun 6, 2019

Author Contributor

Since you can't get the Comorant's Glove outside of the island, I didn't think it was as important as mentioning that no gloves were required; anyone who tries will be told by the nearby Angler that they need to get the glove from them first

Show resolved Hide resolved ...va/net/runelite/client/plugins/cluescrolls/clues/SkillChallengeClue.java Outdated
Show resolved Hide resolved ...va/net/runelite/client/plugins/cluescrolls/clues/SkillChallengeClue.java Outdated
new SkillChallengeClue("Burn a yew log.", item(ItemID.YEW_LOGS), item(ItemID.TINDERBOX)),
new SkillChallengeClue("Catch and cook a swordfish.", "cook a swordfish.", any("Harpoon", item(ItemID.HARPOON), item(ItemID.BARBTAIL_HARPOON), item(ItemID.DRAGON_HARPOON), item(ItemID.INFERNAL_HARPOON), item(ItemID.INFERNAL_HARPOON_UNCHARGED))),
new SkillChallengeClue("Craft multiple cosmic runes from a single essence.", item(ItemID.PURE_ESSENCE)),
new SkillChallengeClue("Plant a watermelon seed.", item(ItemID.RAKE), item(ItemID.SEED_DIBBER), xOfItem(ItemID.WATERMELON_SEED, 3)),

This comment has been minimized.

Copy link
@Nightfirecat

Nightfirecat Jun 6, 2019

Contributor

Rakes aren't needed, particularly if you have auto-weed enabled, or are replacing a freshly-harvested crop.

Suggested change
new SkillChallengeClue("Plant a watermelon seed.", item(ItemID.RAKE), item(ItemID.SEED_DIBBER), xOfItem(ItemID.WATERMELON_SEED, 3)),
new SkillChallengeClue("Plant a watermelon seed.", item(ItemID.SEED_DIBBER), xOfItem(ItemID.WATERMELON_SEED, 3)),

This comment has been minimized.

Copy link
@Hydrox6

Hydrox6 Jun 6, 2019

Author Contributor

There's a surprising amount of people who don't have Auto Weed (because they don't like Tithe Farm), and those who do have it know they have it, so I think it's better to have the extra rake there.

This comment has been minimized.

Copy link
@Nightfirecat

Nightfirecat Jun 6, 2019

Contributor

I'd argue most people would have easy access to a rake via a nearby tool leprechaun, so it seems extraneous to include a tool which is not directly needed, but that's my 2c 🤷‍♂

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch from a85f13b to 0fb1cdc Jun 6, 2019

@Nightfirecat
Copy link
Contributor

left a comment

Code LGTM now

@Nightfirecat Nightfirecat added this to the 1.5.27 milestone Jun 6, 2019

@Adam-

Adam- approved these changes Jun 13, 2019

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Per this comment, it sounds like the existing "Cryptic" clue definition for the text "Kill the spiritual, magic and godly whilst representing their own god." should be removed from the cryptic clue definition and moved to the skill challenge clue definitions? It should work either way, but may be more appropriate here since that's what it technically classifies as.

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch from 0fb1cdc to 5590b80 Jun 17, 2019

@Hydrox6

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Added a commit to remove that mis-categorised clue. There didn't seem to be any others.

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

For completed Sherlock challenge clues, is the clue text struck through? If so, it may be useful to check for that when reading it, so that the plugin could start by reading a completed clue and correctly recognize that it does not still need to be completed.

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

The sherlock clue does not update its text when you read it after completing the challenge. This could induce some strange behavior if one was to get the same challenge clue in the same session, as SkillChallengeClue sets its challengeCompleted value to true, and never resets it.

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Also, the clue overlay is not removed if the clue leaves your inventory, which is unlike other clue scroll types.

@Hydrox6

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Elite Sherlock clues don't strike through once they've been completed, but Master clues do (I think it's that way around).

Using the following should fix the issue of it not being reset, but I'm not sure what negative effects that might have.

if (text.equals(clue.rawChallenge))
{
	clue.setChallengeCompleted(false);
	return clue;
}

The clue not being removed from the inventory is similar to every clue that we don't track the itemID for, such a Beginner clues. It might be worth adding a check for the widget that loads when you get a new clue.

@Hydrox6 Hydrox6 modified the milestones: 1.5.27, 1.5.28 Jun 19, 2019

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Actually, the clue resetting when it is removed from the inventory works fine for beginner clues. It only doesn't work for challenge clues because they don't match this line where the item ID is assigned: https://github.com/runelite/runelite/blob/HEAD@%7B2019-06-21T05:19:31Z%7D/runelite-client/src/main/java/net/runelite/client/plugins/cluescrolls/ClueScrollPlugin.java#L233

I don't remember what the name of these clues is ("Challenge scroll (difficulty)"?) but that's the solution to get the item ID stored.

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch from 5590b80 to 773762d Jun 24, 2019

@Hydrox6 Hydrox6 removed this from the 1.5.28 milestone Jun 26, 2019

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch 2 times, most recently from 4d74ec9 to 6411035 Jul 1, 2019

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

From the SOTE release notes: "One of Sherlock's challenges has had its instructions clarified." Who knows which one it was though. 🤔

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch from 6411035 to 6de9a6d Jul 26, 2019

@Nightfirecat

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Only got to test with one master clue recently ("Burn a magic log.") and it worked just fine for that. I'll post again if I end up with more skill challenge clues.

@Nightfirecat Nightfirecat self-requested a review Aug 6, 2019

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch from 6de9a6d to 7b451d1 Aug 9, 2019

Hydrox6 added some commits Jun 6, 2019

Add support for Challenge Clues
Supports both Sherlock tiers and Charlie

@Hydrox6 Hydrox6 force-pushed the Hydrox6:clue-challenge branch from 7b451d1 to 7244374 Aug 9, 2019

@Adam- Adam- merged commit 6a97d9f into runelite:master Aug 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.