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

[Quest] Implement You Call That a Knife Quest #6372

Open
wants to merge 1 commit into
base: base
Choose a base branch
from

Conversation

jamesbradleym
Copy link
Contributor

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Implements quest You Call That a Knife?

Steps to test these changes

Setup:

  • run dbtool update to get cook mob position update

Pre-Requisites:

  • Khazam Reputation 6+

Quest:

  • Trade a Sandfish to Mhebi Juhbily (!pos 40 -11 -159 250)
  • Trigger the Shed to trigger CS with Mhebi Juhbily -- Quest Start
  • Talk to Vah Keshura (!pos 30 -8.5 -105 250)
  • Trade a Tonberry Board to Chef Nonberry (!additem 1145) (!pos -136 0.0 -91 159)
  • Talk to Mhebi Juhbily with Key Item Nonberry's Knife -- Quest Complete

Notes:

  • It is not required to kill the mobs spawned by a bad trade to Chef Nonberry, you can trade the correct item at any time to get the Key Item
  • The Shed does not seem to have a post quest + zone event
  • Spawned Mob positions are estimated from the video capture (It looks like the capture cut out too soon so mob positions were not saved out?)
  • Capture showed 10min repop for mobs
  • Capture shows new default event for Mehbi Juhbily post quest and zone
  • Mobs can only be popped while on quest

Capture

@jamesbradleym jamesbradleym changed the title Implement You Call That a Knife Quest [Quest] Implement You Call That a Knife Quest Oct 18, 2024
Copy link
Contributor

@dallano dallano left a comment

Choose a reason for hiding this comment

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

Other than the comments, this PR looks great to me

[28] = function(player, csid, option, npc)
player:confirmTrade()
if os.time() >= npc:getLocalVar('cooldown') then
spawnCooks(player, npc)
Copy link
Contributor

Choose a reason for hiding this comment

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

There exists a command for this in npcUtil:
npcUtil.popFromQM(player, npc, { TABLE_OF_MOD_IDS }, {claim = boolean, hide = integer}) etc. etc.

if npcUtil.tradeHasExactly(trade, { xi.item.TONBERRY_BOARD }) then
return quest:progressEvent(27)
else
npcUtil.confirmTradedItems(trade) -- Take all items traded
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create a function for this? You should be able to just use player:confirmTrade()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding player:confirmTrade() works on items that have been confirmed (such as through npcUtil.tradeHasExactly), I did not see a utility that would accept all items regardless of confirmation so I made this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave that discretion with LSB mods.

Comment on lines 178 to 180
quest:complete(player)
player:delKeyItem(xi.ki.NONBERRYS_KNIFE)
quest:setMustZone(player)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a safe practice to get into. If there's an error when trying to complete the quest, the player won't lose their key item. This is more common and useful with quests that have items in their rewards

Suggested change
quest:complete(player)
player:delKeyItem(xi.ki.NONBERRYS_KNIFE)
quest:setMustZone(player)
if quest:complete(player) then
player:delKeyItem(xi.ki.NONBERRYS_KNIFE)
quest:setMustZone(player)
end

Comment on lines 11 to 16
xi.mix.jobSpecial.config(mob, {
specials =
{
{ id = xi.jsa.ASTRAL_FLOW },
},
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't required. It's done by loading the mixin above like you've done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, gotcha. Thanks for the explanation!

xi.mix.jobSpecial.config(mob, {
specials =
{
{ id = xi.jsa.MIJIN_GAKURE },
},
})
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for the others

end

local function spawnCooks(player, npc)
if not isFightInProgress() then
Copy link
Contributor

Choose a reason for hiding this comment

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

popFromQM makes this check
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got distracted by the QM part 😅
Appreciate the review here! Removed the other bits as they're no longer necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Thanks for the quick responses here.

@jamesbradleym jamesbradleym force-pushed the You-Call-That-a-Knife branch 3 times, most recently from a6ca7d0 to af73a26 Compare October 19, 2024 03:49
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