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]Distant Loyalties and associated quests #5643

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

Conversation

hooksta4
Copy link
Contributor

@hooksta4 hooksta4 commented May 7, 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?

Implements sandy quest Distant Loyalties

  • adds checks to quests involving NPC's for distant loyalties (blocks other quests from being accepted if on distant loyalties)
  • The Elvaan Goldsmith, Father Figure.
  • adds default dialog to sandy NPC
  • Removes npc lua functions associated.
  • Added check for repeat of The Elvaan Goldsmith once The Return of the Adventurer is completed.

Capture

Steps to test these changes

Get Fame 4 in sandy. Speak with Femitte accept quest. Travel to Bastok Markets, NPC Michea will not give Father Figure or The Elvaan Goldsmith until completed.
Complete quest and then can accept The elvaan goldsmith and father figure.

@@ -20,7 +20,9 @@ quest.sections =
check = function(player, status, vars)
return status == xi.questStatus.QUEST_AVAILABLE and
player:hasCompletedQuest(xi.questLog.BASTOK, xi.quest.id.bastok.THE_ELVAAN_GOLDSMITH) and
player:getFameLevel(xi.fameArea.BASTOK) >= 2
player:getFameLevel(xi.fameArea.BASTOK) >= 2 and
player:getQuestStatus(xi.questLog.SANDORIA, xi.quest.id.sandoria.DISTANT_LOYALTIES) ~= xi.questStatus.QUEST_ACCEPTED
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is really the case, or if we should be using event priority more effectively. Not a problem for now, but a bigger discussion in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The capture wouldn't let me flag quest while I had it loaded. Uses the same NPC - spoke to it multiple times, but I agree there might be some priority for others...

@@ -40,7 +42,8 @@ quest.sections =
(
status == xi.questStatus.QUEST_COMPLETED and
player:getFameLevel(xi.fameArea.BASTOK) == 1 and
not quest:getMustZone(player)
not quest:getMustZone(player) or
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic looks a bit loose by tacking that or on in the end of 3 and conditions. Even if not necessary, it should probably be clarified with parens

{
check = function(player, status, vars)
return status == xi.questStatus.QUEST_AVAILABLE and
player:getFameLevel(xi.fameArea.SANDORIA) >= 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Please indent for this line


quest.reward =
{
fame = 30,
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 styling, can you please align the ='s

['Femitte'] =
{
onTrigger = function(player, npc)
local questProgress = quest:getVar(player, 'Prog')
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to define this as a var here, since its only used in one of the conditions

{
onTrade = function(player, npc, trade)
if
npcUtil.tradeHasExactly(trade, xi.item.MYTHRIL_INGOT) and
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent needed

onEventFinish =
{
[315] = function(player, csid, option, npc)
player:delKeyItem(xi.ki.GOLDSMITHING_ORDER)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this silent on retail or displayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Silent

[317] = function(player, csid, option, npc)
player:confirmTrade()
quest:setVar(player, 'Prog', 2)
player:needToZone(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Convention is to use the quest's setMustZone function. needToZone is too generic.

[Quest]Distant Loyalties and associated quests

[Quest]Distant Loyalties and associated quests
@hooksta4
Copy link
Contributor Author

@claywar - made the changes you requested.

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.

None yet

2 participants