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

[lua&sql] Alterations to Trust: Monberaux logic and spells. Added trade for Elixir and Gil. #6950

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

Conversation

shigukahz
Copy link
Contributor

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?

Changes made to Trust: Monberaux and Monberaux in Upper Jeuno for the follow;
Added Elixir/Hi-Elixir and Gil trading to his NPC in Upper Jeuno to allow the function of his AoE status removal and his Final Elixir mechanics.

Changed his Trusts mJob and sJob to PLD/RUN to reflect his status as shown on BGWiki.

Edited his spells so that if they don't remove the effect they show a "No effect on player" message, mainly only matters for AoE potions.

Changed SQL for mob_skills to correct some animations and make one group of the status removal spells AoE and the other not.

Added 2 new spell timers into his logic, he's meant to cast Mix I potions on a 3-4 second intervals, the Mix II on 60 second intervals, and Mix III at 90 seconds. Currently he is doing all spells on 3 second intervals, this should correct and bring him closer to retail timing.

Renamed most of his skills in mobskills to have mix_ in front to hopefully avoid confusion with other potential applications. All spells range changed from 7 to 14 to accommodate his "NO_MOVE" trust behaviour, he would previous stand out of random and continuously loop some spells because he couldn't reach. No his spells are more in like with White Magic range.

Steps to test these changes

To test his NPC to see how it handles the trading of Gil and Elixirs: I have tried many different trade combinations to confirm the correct ones I was aiming for worked for each CVar. Using !checkvar Player "monbAoe" and !checkvar Player "finalElixir" I was able to confirm the trades for each section were working correctly.

To test his Trust: To test his on spawn text logic, I used !setplayervar to try the combinations required to get all the text outputs, ensuring each one was given when needed.

Tested all of his status removal spells by having an effect added onto my character and see how he responds, with both AoE setting on and off.

@shigukahz shigukahz marked this pull request as ready for review February 4, 2025 22:27
@shigukahz shigukahz marked this pull request as draft February 4, 2025 22:48
@Mortalelite
Copy link
Contributor

Please don't use Tab when making new lines, would need to convert them to spaces.

@shigukahz
Copy link
Contributor Author

Please don't use Tab when making new lines, would need to convert them to spaces.

Understood thank you, I've converted to draft - because even though all is working server side, I need to clean up the errors, going through them currently.

@shigukahz shigukahz force-pushed the base branch 2 times, most recently from 7841646 to d2d8aca Compare February 4, 2025 23:50
INSERT INTO `mob_skills` VALUES (4259,2610,'mix_dragon_shield',1,0.0,7.0,2000,100,3,4,0,0,0,0,0); -- verified, but no effect messaging is probably wrong
INSERT INTO `mob_skills` VALUES (4260,2611,'mix_dark_potion',0,0.0,7.0,2000,100,3,4,0,0,0,0,0); -- verified, but check if correct messaging.
INSERT INTO `mob_skills` VALUES (4261,2612,'mix_samsons_strength',1,0.0,7.0,2000,100,3,4,0,0,0,0,0); -- verified, but check no effect messaging
INSERT INTO `mob_skills` VALUES (4231, 2613, 'mix_final_elixir', 0, 0.0, 14.0, 2000, 1000, 3, 4, 0, 0, 0, 0, 0); -- Trust: Monberaux Final Elixir
Copy link
Contributor

Choose a reason for hiding this comment

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

No spaces
INSERT INTO `mob_skills` VALUES (4231, 2613, 'mix_final_elixir', 0, 0.0, 14.0, 2000, 1000, 3, 4, 0, 0, 0, 0, 0); -- Trust: Monberaux Final Elixir

Should look like this
INSERT INTO `mob_skills` VALUES (4231,2613,'mix_final_elixir',0,0.0,14.0,2000,1000,3,4,0,0,0,0,0); -- Trust: Monberaux Final Elixir

That is how the other mob_skills is setup

Changes made to Trust: Monberaux and Monberaux in Upper Jeuno for the follow;
Added Elixir/Hi-Elixir and Gil trading to his NPC in Upper Jeuno to allow the function of his AoE status removal and his Final Elixir mechanics.

Changed his Trusts mJob and sJob to PLD/RUN to reflect his status as shown on BGWiki.

Edited his spells so that if they don't remove the effect they show a "No effect on player" message, mainly only matters for AoE potions.

Changed SQL for mob_skills to correct some animations and make one list of the status removal AoE and the other not.

Renamed most of his skills in mobskills to have mix_ in front to hopefully avoid confusion with other potential applications. All spells range changed from 7 to 14 to accommodate his "NO_MOVE" trust behaviour, he would previous stand out of random and continuously loop some spells because he couldn't reach. No his spells are more in like with White Magic range.
@shigukahz shigukahz marked this pull request as ready for review February 5, 2025 15:14
@shigukahz shigukahz requested a review from Mortalelite February 5, 2025 15:51
then
if finalElixir < elixirTotal then
player:setCharVar('finalElixir', finalElixir + 1)
player:printToPlayer(string.format('Thank you for that medicine. You currently have %d vials remaining.', finalElixir + 1), 0, 'Monberaux')
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually how SE does this? it's not a cutscene but message packets?

Copy link
Member

Choose a reason for hiding this comment

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

image
Nope, it's real CS events, I took a cap so I can try to figure out params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure myself of the exacts of it - I've not unlocked him yet cause of the high deed requirement, I'm at like 360, so added it in that way, so it was in place for future improvement.

Copy link
Member

Choose a reason for hiding this comment

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

whoops, I forgot to start the capture

@zach2good
Copy link
Contributor

Here are captures I made last year for this process:

Monberaux gil and Elixer trading - Trust Monberaux fighting and Lhe Lhangavo fighting
Video: https://drive.google.com/open?id=17zImydejvFKIsUBt4Y6BQkcyF2MUCFVS
Packets: https://drive.google.com/open?id=1M3l8PTIVU6uigrekf9niTofT1PZahahR

@shigukahz
Copy link
Contributor Author

Here are captures I made last year for this process:

Monberaux gil and Elixer trading - Trust Monberaux fighting and Lhe Lhangavo fighting Video: https://drive.google.com/open?id=17zImydejvFKIsUBt4Y6BQkcyF2MUCFVS Packets: https://drive.google.com/open?id=1M3l8PTIVU6uigrekf9niTofT1PZahahR

If I'm understanding correctly, the IDs for the event are 'No gil donation' = 10237, 'Performing gil donation' = 10238, 'has gil donation' = 10239 || and in same order for elixir, 10241, 10243, 10244?

@WinterSolstice8
Copy link
Member

Do you think you're able to handle adding the events? If not we could punt it for later. We don't really want to add printToPlayer messaging because it's not accurate

@shigukahz
Copy link
Contributor Author

shigukahz commented Feb 9, 2025

Do you think you're able to handle adding the events? If not we could punt it for later. We don't really want to add printToPlayer messaging because it's not accurate

I'm not confident I could accurately add all the startEvents and get the params correct to be honest. Haven't touched on those yet at all. EDIT: I may have figured it out, I will test full later and let you know.

Swapped out the printToPlayer aspects and adding the start events. 

Will accept the item, and correctly say thank you for the correct itemID. 

If trying to trade when his max cvars are reached, will play appropriate csid.

Max Elixirs and Minimum Gil entered as variables so they can be changed per server needs.
@shigukahz
Copy link
Contributor Author

Do you think you're able to handle adding the events? If not we could punt it for later. We don't really want to add printToPlayer messaging because it's not accurate

I have updated the NPC file for Monberaux now, working on my local server with events rather than prints, for both items and the gil. Does the CVar used matter, or are the ones I've chosen okay?

if
player:getCharVar('monbAoe') == 0
then
player:startEvent(10237, 100000, 1, 2964, 3300, 58195966, 3881063, 4480, 128)
Copy link
Member

@WinterSolstice8 WinterSolstice8 Feb 12, 2025

Choose a reason for hiding this comment

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

can you leave a comment here saying the 2nd param of 100k is how much gil he asks for and is hardcoded on retail? see Coco's comment blelow

Copy link
Contributor Author

@shigukahz shigukahz Feb 12, 2025

Choose a reason for hiding this comment

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

~can you leave a comment here saying the 2nd param of 100k is how much gil he asks for and is hardcoded on retail?
~ see Coco's comment blelow

Have altered the coding for Coco's comment, but PR is failing on squash at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

The squash is a reminder to us before we merge the rest of the CI will run without it

Copy link
Member

@WinterSolstice8 WinterSolstice8 Feb 12, 2025

Choose a reason for hiding this comment

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

 scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:22: Found tab character(s) in file, please replace these with 4x spaces.
scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:23: Found tab character(s) in file, please replace these with 4x spaces.
scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:24: Found tab character(s) in file, please replace these with 4x spaces.
scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:25: Found tab character(s) in file, please replace these with 4x spaces.
scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:62: Found tab character(s) in file, please replace these with 4x spaces.
scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:63: Found tab character(s) in file, please replace these with 4x spaces.
scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:64: Found tab character(s) in file, please replace these with 4x spaces.
scripts/zones/Upper_Jeuno/npcs/Monberaux.lua:65: Found tab character(s) in file, please replace these with 4x spaces.

Just gotta replace some tabs with spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah going back to do them now, apologies.

@WinterSolstice8 WinterSolstice8 added the squash Reminder to squash commits before/on merge (you can do this, or maintainers will do this for you) label Feb 12, 2025
Comment on lines 13 to 18
local minimumGil = 100000
local elixirTotal = 2

local finalElixir = player:getCharVar('finalElixir')
local monbAoe = player:getCharVar('monbAoe')
local gilAmount = trade:getGil()
Copy link
Contributor

Choose a reason for hiding this comment

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

Monberaux wants 10% of your gil, with a minimum of 10,000 and a maximum of 100,000, not a set amount.

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, so you can game the system?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I always trade most of my gil to my alt and leave 100k. Wiki mentions using the Mog Gardens chest for storage too.

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'm currently testing on my local;

local minimumGil = player:getGil() * 0.1
if minimumGil > 100000 then
minimumGil = 100000
elseif minimumGil < 10000 then
minimumGil = 10000
end

Seems to work so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference, this could be simplified as:

local minimumGil = utils.clamp(player:getGil() * 0.1, 10000, 100000)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there's a lot of new if statements in his script that will both be firing startEvent, is that right? Like consider the onTrade function. It will start event 10243 or 10246 AND 10238 or 10240. There's similar things in onTrigger and onEventFinish.

    if
        player:getCharVar('monbAoe') == 0
    then
        player:startEvent(10237, minimumGil, 1, 2964, 3300, 58195966, 3881063, 4480, 128)
    else
        player:startEvent(10239)
    end

    if
        player:getCharVar('finalElixir') <= elixirTotal
    then
        player:startEvent(10241)
    else
        player:startEvent(10244)
    end

What happens when you trigger him? This looks like it's always trying to do the gil and the elixir events. It looks like he's supposed to cycle through his dialog each time you trigger him, which I think calls for interaction framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On first interaction, he plays his default !cs 28, then on second interaction with him, he plays 2 lines in one chat, 1 for each event.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you check Zach's capture above, he does 1 event each time he's clicked. 10237, 8, 10241, 169, 10237... This should probably be set up as a hidden quest so these lines get cycled with the rest of his quest lines. Is any of this applicable if you don't have him as a trust? The quest check function could handle that and I think the interaction framework handles the dialog cycling. If you do the quest Save the Clock Tower or The Lost Cardian, is he asking for gil and elixirs after unrelated quest lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you check Zach's capture above, he does 1 event each time he's clicked. 10237, 8, 10241, 169, 10237... This should probably be set up as a hidden quest so these lines get cycled with the rest of his quest lines. Is any of this applicable if you don't have him as a trust? The quest check function could handle that and I think the interaction framework handles the dialog cycling. If you do the quest Save the Clock Tower or The Lost Cardian, is he asking for gil and elixirs after unrelated quest lines?

I'll check about the questlines but as a fresh character with no trusts, his dialog is the same as it is with my character that has him as a trust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Triggered the requirements for the following, and he played out all 3 events at the same time, the one from the lua, and the 2 for his trust logic.

    elseif
        theLostCardien == xi.questStatus.QUEST_COMPLETED and
        player:getQuestStatus(xi.questLog.JEUNO, xi.quest.id.jeuno.THE_KIND_CARDIAN) == xi.questStatus.QUEST_ACCEPTED
    then
        player:startEvent(32)
    end

Changed how much gil he asks the player for to be 10% of their current gil, with a threshold of 10,000 min and 100,000 max.
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.

5 participants