server icon indicating copy to clipboard operation
server copied to clipboard

Migrate Expert Craft Quest Dialog Workaround to ENUMs

Open neuromancerxi opened this issue 3 years ago • 3 comments

I affirm:

  • [x] I have paid attention to this example and will edit again if need be to not break the formatting, or I will be ignored
  • [x] I have read and understood the Contributing Guide
  • [x] I've 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?

  • Updates the Expert Craft Quest Dialog workaround introduced in e850dc2bae993d7c6cfee541cf4308ffc730cd67 from Magic Numbers to ENUMs to prevent dialog drift during future version updates.
  • Properly displays dialog with Key Items and Items as part of inline text.

Steps to test these changes

  1. Log in an appropriate test GM character
  2. Visit any Guild NPC
  3. !setskill <craftskill> 98
  4. !setcraftrank <craftskill> 9
  5. Follow dialogs from Guild Leader
  6. !addkeyitem way_of_the_<name>
  7. Note Dialog no longer presents unrelated gibberish.

image

neuromancerxi avatar Jul 25 '22 21:07 neuromancerxi

Is there a cap for this? I'd be a bit surprised if retail doesn't use an event for that series of messages

claywar avatar Jul 25 '22 22:07 claywar

Is there a cap for this?

I was unable to locate one when I did the initial PR in 2020, nor when I submitted this recent correction to prevent drift in the dialog.

I'd be a bit surprised if retail doesn't use an event for that series of messages

As far as I'm aware, it uses the same CSID that the rest of the lower tiers use, just with different params. Currently, feeding the proper parameters will cause the the client to hang in cutscene, which is why I opted for the use of a workaround.

This may possibly be due to an unimplemented packet or function (display recipe?). The Workaround is to present dialog to player to let them know the trade is ready to be received by feeding raw text using showText instead. This workaround can be removed once the packets no longer hang the client.

Similar comments are present in each of the Guild Leader scripts describing why the workaround is being used, how it functions, and the conditions where it won't be needed and can be removed.

This behavior can be observed by commenting out the work around, and talking to the NPC to observe the client hang.

neuromancerxi avatar Jul 26 '22 12:07 neuromancerxi

Following up on this PR.

There is a version update scheduled for August 10th, which will potentially include ID Shifts. Would it be possible to review/provide feedback on this PR ahead of then so the newly tracked IDs included within can be included with the automated crawl that's done with the regular version update?

Thanks in advance!

neuromancerxi avatar Aug 04 '22 15:08 neuromancerxi

@neuromancerxi I wonder if #2649 might help with this. Going to investigate to see if there's a request here that needs to be handled separately.

claywar avatar Sep 04 '22 14:09 claywar

Local testing in the branch mentioned above: image

claywar avatar Sep 04 '22 14:09 claywar

image

@claywar Looks like #2649 may have resolved the client hang, thanks! I'll run through the guild leaders and test out the Expert dialog, and submit a PR to incorporate the proper dialog.

neuromancerxi avatar Sep 05 '22 19:09 neuromancerxi