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 X Marks the Spot #6367

Open
wants to merge 3 commits into
base: base
Choose a base branch
from

Conversation

slashtangent
Copy link
Contributor

  • 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?

Add the quest "X Marks the Spot" and removes unused code from _0r9.lua

Steps to test these changes

-- Tavnazian Safehold !zone 26
Talk to Despachiaire
-- Despachiaire !pos 111.2090 -40.0148 -85.4810
Talk to Parelbriaux
-- Parelbriaux !pos 113.701 -41 42.3653
Talk to Odeya
-- Odeya !pos 83.3227 -34.25 -70.0384
Trade Tavanzian Liver to Odeya !additem 5154
-- Phomiuna Aqueducts !zone 27
Check Gate:
-- Hidden Hallway 2nd Gate !pos 138.1027 -24 60.3209
-- Tavnazian Safehold !zone 26
Talk to Odeya to end quest.
-- Odeya !pos 83.3227 -34.25 -70.0384

Sources

Retail Capture:
Video: https://drive.google.com/open?id=10Jvy5TKRKbDSbq0_ku5hObid4AsBlvDD
Packets: https://drive.google.com/open?id=1-LEXYfCeie3kzkBpwH68jyg7pItTeE1c

Additional Capture:
Part 1: https://youtu.be/bH8bb61KUdo
Part 2: https://youtu.be/FOxpWM70ea4
https://1drv.ms/u/s!AlLEVQXhZeaHgV628MzKgxRQydEL?e=Bw04wF

* Added X Marks the Spot
* Added item Tavnazian Liver
* Removed unused code from _0r9
Copy link
Contributor

@zach2good zach2good left a comment

Choose a reason for hiding this comment

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

Seems pretty reasonable to me 👍

Comment on lines 98 to 99
player:confirmTrade()
quest:setVar(player, 'Prog', 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are double-indented compared to the other blocks nearby

if quest:getVar(player, 'Prog') == 3 then
return quest:progressEvent(37)
else
return
Copy link
Contributor

Choose a reason for hiding this comment

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

You can get rid of the else-return here if it's going to do nothing. Presumably the ??? or whatever this is has a DefaultAction set up in the file for that zone?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you've changed this and it falls out the bottom, should this 0r9 thing have a default action? What's in the cap?

local ID = zones[xi.zone.PHOMIUNA_AQUEDUCTS]

return {
    ['_0r5'] = { messageSpecial = ID.text.DOOR_FIRMLY_SHUT },
}

Copy link
Contributor Author

@slashtangent slashtangent Oct 18, 2024

Choose a reason for hiding this comment

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

In the capture, the door doesn't open and gives a line about the door being firmly shut. I've been doing research to see if there is any instance in which this door opens as it is coded (outside of the cutscene). I was going to put in a capture request to have someone test it again outside of the quest.

I suspect that the door isn't meant to open outside of the quest, but I want to verify since the code for the door opening was added way back in 2020.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just watched the cap, all the information needed is there. It plays you this message if there isn't a quest involved:
image

So set that as the default action for this door. (there may have been some ID shifts since this cap was made, so figure it out with your local client)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm suspicious about the doors around here, because they all have this block:

    elseif npc:getAnimation() == 9 then
        npc:openDoor()
    end

I don't remember the story around this area, if gates are meant to be shut at any time, if you need keys, etc. This might just be copy/paste error, or they might legitimately need to open - and that's hard to figure out from caps

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect a retail character with large mission completion could quickly go there and see what the door does, but then you'd have to work backwards from "it's normally open, when is it shut?" or "it's normally shut, when is it open?"

Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of this PR then, nothing needs to happen, the underlying object opens like it always has, and you don't change that behaviour 👍

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 think you're right and it might just be a copy-paste error. @siknoz did a recent capture (almost a year after the x marks the spot capture) for me to double-check the door and it still displays the door is firmly shut. I also couldn't find any instance where the player needs to go into the room beyond here.

image

I'm going to go ahead and add in the default action and remove the door opening animation, if it is ever found to have a need then it can be added back.

@@ -21,9 +19,6 @@ entity.onEventUpdate = function(player, csid, option, npc)
end

entity.onEventFinish = function(player, csid, option, npc)
if csid == 37 then
player:setCharVar('X_MARKS_THE_SPOT', 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that these X_MARKS_THE_SPOT vars are completely vestigial, and not set/used anywhere else 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added to the code way back in 2020. My best guess is that someone was working on it but eventually scrapped it.

@zach2good zach2good added the squash Reminder to squash commits before/on merge (you can do this, or maintainers will do this for you) label Oct 18, 2024
@slashtangent
Copy link
Contributor Author

@zach2good checking in to see if there is anything else needed here.

{
check = function(player, status, vars)
return status == xi.questStatus.QUEST_AVAILABLE and
player:getCurrentMission(xi.mission.log_id.COP) > xi.mission.id.cop.ANCIENT_VOWS
Copy link
Contributor

Choose a reason for hiding this comment

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

Styling a bit here, please indent returns after the first line

[xi.zone.TAVNAZIAN_SAFEHOLD] =
{
-- Will repeat until the quest is completed.
['Despachiaire'] = quest:progressEvent(144),
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 confirmed to be blocking? Not sure what else he could be handling, and would be fine assuming all of his other interactions are progress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewatching the capture it does not block. However, it is one of the scenes he cycles through depending on what you have active, and this cs will get cycled in until you complete the quest.

I'll need to recode this. However, I'm unsure how to go about making sure it still cycles in other scenes available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use quest:event(x)

If the other quests he is involved in are properly coded using the priority method, the cycle will happen automatically.

:progressEvent() means the event takes priority over other events of lower priority. :event() has lower priority, for instance.

If 2 events have the same priority, the cycling happens automatically.

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

Choose a reason for hiding this comment

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

Please pull and assign a variable once instead of querying for every condition

@zach2good zach2good added squash Reminder to squash commits before/on merge (you can do this, or maintainers will do this for you) and removed squash Reminder to squash commits before/on merge (you can do this, or maintainers will do this for you) labels Nov 7, 2024
@slashtangent
Copy link
Contributor Author

@claywar @Xaver-DaRed Checking in on this one.

* Added X Marks the Spot
* Added item Tavnazian Liver
* Removed unused code from _0r9 and added default action to match captures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squash Reminder to squash commits before/on merge (you can do this, or maintainers will do this for you)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants