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] An Affable Adamantking #5207

Conversation

jamesbradleym
Copy link
Contributor

@jamesbradleym jamesbradleym commented Mar 1, 2024

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?

Closes: HorizonFFXI/HorizonXI-Issues#1787

Allows players to accept and complete the quest An Affable Adamantking.

https://www.youtube.com/watch?v=KutqDTqCqM8/

Steps to test these changes

Update your db with dbtool to import mob spawn changes.

Zone in to Qulum Dome from Beadeaux H-7, with minimum level 60, and no other beastmen hat quest active. (If you decline, you must wait until the next conquest tally)

Go to Peshi Yohnts in Windurst Woods to get Quadav Parts by trading in a Turtle Shell, Bugard Leather, and 10,000 gil. Zone and wait until next Vana day.

Return to Qulum Dome from Beadeaux H-7 with Quadav Barbut equipped.

Go to Beastmen's Banner !pos 10 25 -50 148

Kill Diamond Quadav - Quest complete

Zone in and out of Qulum Dome with D'ahvus's Barbut equipped for additional CS and gold beast coin reward

Notes

Resources:
https://ffxi.allakhazam.com/db/quests.html?fquest=657
http://ffxi.somepage.com/questdb/289
http://www.playonline.com/pcd/update/ff11us/20071120wwnX41/detail.html
https://ffxi.allakhazam.com/db/item.html?fitem=9375

-- !addItem 1637 -- Bugard Leather
-- !addItem 15201 -- Quadav Barbut
-----------------------------------
require('scripts/globals/npc_util')
Copy link
Contributor

Choose a reason for hiding this comment

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

This require is unnecessary, globals are preloaded on server start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

['Peshi_Yohnts'] =
{
onTrigger = function(player, npc)
if quest:getVar(player, 'Prog') == 0 then
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be reading Prog value every time (for clarity), better to define a variable prior to the condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored so prog is a local variable read once 😅

{
['Peshi_Yohnts'] =
{
onTrigger = function(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.

Looks like everything here is returning a progressEvent. Is this intentional? That means this quest would be blocking potentially for anything else (unless it also has the same priority level return)

Copy link
Contributor Author

@jamesbradleym jamesbradleym Mar 9, 2024

Choose a reason for hiding this comment

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

A bit over zealous with the progressEvents, the status/check-in dialogues could be event(#):importantOnce to not be blocking 👍🏻

end

quest:setVar(player, 'partsWait', 0)
npcUtil.giveItem(player, xi.item.QUADAV_PARTS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can fail, and needs to be attempted and checked prior to changing other variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

player:hasKeyItem(xi.ki.ORCISH_SEEKER_BATS)
then
if
GetNPCByID(qulunDomeID.npc.BEASTMENS_BANNER):getLocalVar('engaged') < os.time() and -- if engaged 3min timer has passed
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're checking the same local var twice, it would be more clear to set it in a variable prior to the condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored so engaged is a local variable read once

[64] = function(player, csid, option, npc)
if player:getFreeSlotsCount() > 0 then
player:confirmTrade()
npcUtil.giveItem(player, xi.item.DAVHUS_BARBUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

This reward can be covered by the reward table, and then check if quest:complete(player). This will also help to ensure that the player successfully received the item prior to setting other values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved reward to reward table and nested all prog/var setting into quest:complete(player) check

onEventFinish =
{
[65] = function(player, csid, option, npc)
if player:getFreeSlotsCount() > 0 then
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified, as I believe giveItem will also send the appropriate message if the player cannot receive the item. if npcUtil.giveItem(player..) then set charvar to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to use giveitem🤘

@@ -34,6 +34,7 @@ zones[xi.zone.QULUN_DOME] =
},
npc =
{
BEASTMENS_BANNER = 17383484,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for some shift-proofing, GetFirstID('Beastmens_Banner')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, works for me!

@jamesbradleym jamesbradleym force-pushed the An_Affable_Adamantking branch 2 times, most recently from 0ecae2f to 8c179cd Compare March 9, 2024 02:17
@jamesbradleym jamesbradleym requested a review from claywar March 9, 2024 02:17
@jamesbradleym
Copy link
Contributor Author

@claywar Thank you for taking the time to review these PRs, I greatly appreciate it!

git commands make me nauseous

@jamesbradleym jamesbradleym force-pushed the An_Affable_Adamantking branch from 8c179cd to 7e333e7 Compare March 26, 2024 23:50
@jamesbradleym jamesbradleym force-pushed the An_Affable_Adamantking branch from 7e333e7 to 7db75ec Compare April 8, 2024 01:09
@jamesbradleym jamesbradleym force-pushed the An_Affable_Adamantking branch 2 times, most recently from 637ed56 to 7723a7d Compare May 4, 2024 02:47
@jamesbradleym jamesbradleym force-pushed the An_Affable_Adamantking branch from 7723a7d to 869ab26 Compare May 15, 2024 21:18
@jamesbradleym jamesbradleym force-pushed the An_Affable_Adamantking branch from 869ab26 to 7c5c46c Compare May 31, 2024 23:12
@jamesbradleym
Copy link
Contributor Author

@claywar Checking in on this 😅

@jamesbradleym
Copy link
Contributor Author

Hey @claywar Sorry to bump this one again, let me know if anything else is missing!

@jamesbradleym jamesbradleym force-pushed the An_Affable_Adamantking branch from 7c5c46c to e666015 Compare July 15, 2024 21:56
@jamesbradleym
Copy link
Contributor Author

Closing this PR to reduce open PRs and prevent staleness—will revisit.

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.

🐛[BUG][Quest] An Affable Adamantking?
2 participants