server icon indicating copy to clipboard operation
server copied to clipboard

[lua] Quest Audit

Open slashtangent opened this issue 8 months ago • 3 comments

I affirm:

  • [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • [x] I have read and understood the Contributing Guide and the Code of Conduct.
  • [x] 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?

Audits the quest.lua enum for quests converted to IF not marked. Marks a few quests found coded outside of IF, not marked. Marks quests with optional modules.

Steps to test these changes

N/A

slashtangent avatar Apr 16 '25 02:04 slashtangent

The point of the module system is that customisation and era-rollbacks don't need to leak into the main retail codebase. Marking things as "there's an optional era module for this" doesn't do anything. If people want to look at the era stuff, they can look in modules/era

While you're in here can you choose between ± and + and make all of the markers use that? They're currently mixed

The point of marking the quests with modules is to have an easy visual of which quests have modules. One of the two modules in this case isn't an ERA module. But a module to give the quest retail accurate functionality. By default, the quest is missable once the player reaches fame 3+ in Bastok. This module allows server owners to keep the quest missable or not. https://github.com/LandSandBoat/server/pull/3992

This also stands as a visualization for contributors and server owners to easily gauge what is coded and what has been converted to IF. I don't see any reason not to mark what quests have modules.

As far as ± and +, I originally did standardize everything. However, after talking with @KnowOne134 these symbols had a purpose at one time. Quests not marked as converted tagged with ± indicated that the quest is retail verified and tested. + is best effort. I think it's still worth keeping this around as a visual for contributors, but I don't see any concern standardizing the IF quests since they are supposed to be retail accurate and tested.

slashtangent avatar Apr 16 '25 10:04 slashtangent

In these cases I don't think marking things with + Optional Module is useful, or good enough to convey information.

In the case of Northward: this doesn't need marking. Era modules don't need to pollute the main codebase. They only do for legacy or C++ purposes.

In the case of 'Mom', it could have a longer comment explaining why we've deviated from retail:

MOM_THE_ADVENTURER = 21, -- + Converted, NOTE: This is missable on retail. We've opted not to follow that behaviour. There is a module in <location> to restore that behaviour if desired 

We were talking internally and these are the following cases for marking:

-- +
-- ±
-- + Converted
-- ± Converted

When what we're trying to communicate is:

Implemented:
The quest plays out as some fair approximation of what the wikis say.

Verified:
Has been exhaustively tested against retail captures.

Converted:
"Original" implementation in NPC files has been removed and replaced with Interaction Framework files. Implies "Implemented", but doesn't necesserily imply "Verified".

Currently, the marking is tribal knowledge and after 6 years I didn't know what they meant, so they're useless to observers. At the very very least they need to be explained at the top of the file. Though, the "verified"-ness of these quests is up for debate, and it might be worth only marking things that are fully and properly implemented and verified in IF.

zach2good avatar Apr 17 '25 11:04 zach2good

For converted quests I think we should just mark them as simply -- Converted and remove the + and ± altogether on quests converted to IF. Whoever converted them is supposed to make sure they are retail accurate and tested before PRing.

There is still merit in having the non-IF quests marked, just so people are aware they they are coded in some capacity. These markings will get removed as quests are converted to IF. Thought I do agree about the questionable nature of the verifiedness of quests marked with ±. I don't think I've worked on a single one that wasn't missing at least some interaction.

I think we can just knock this down to two markings. -- + coded in non IF form and -- Converted. Then at the top we can easily indicate what each means:

-----------------------------------
--- +         : The quest is coded and completeable but might not be fully fleshed out and accurate.
--- Converted : The "Original" implementation in NPC files has been removed and replaced with Interaction Framework files. Implies "Implemented", but doesn't necesserily imply "Verified".
-----------------------------------

slashtangent avatar Apr 17 '25 16:04 slashtangent