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

[Avatar] Implement Cait Sith #4625

Merged
merged 1 commit into from
Nov 12, 2023
Merged

[Avatar] Implement Cait Sith #4625

merged 1 commit into from
Nov 12, 2023

Conversation

MowFord
Copy link
Contributor

@MowFord MowFord commented Oct 20, 2023

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 the Cait Sith smn avatar!

I'm starting this off as a draft to get feedback on the core changes I've had to make for Level ? Holy, as well as the path I've taken for the pet skills. Specifically, the buff spells don't report in the chat log properly

I'm running into a weird situation with using xi.msg.basic.SKILL_GAIN_EFFECT when these are mobskills. It makes sense, as it's likely reporting the player ability ID as the mob skill ID. If i set the pet_skill_finish_id as player JA finish, then the chat log reports properly but the animation is wrong <.<

I've got the buff abilities to work via xi.msg.basic.IS_STATUS, but it isn't the cleanest.

Note that I've not adjusted the meat of the offensive abilities to actually function, I'm merely making sure I'm on the right track here. It will technically work this way, but I want to make sure it's the way LSB would prefer.

Level ? Holy working via clay's adjustments:
This was a level 5, animation was proper and it properly had no effect on the mob that wasn't divisible by 5
image

Onto a structured list of the changes:

  • Cait sith has proper name prefix and named properly to be "Cait Sith" instead of "The CaitSith"
    • Done via proper name in pet_list and name_prefix in mob_pools set to 32
  • abilities enabled in abilities.sql and set to have proper MP cost (and other metadata)
    • Also fixed a bug with one of fenrir's abilities being grouped in when Cait Sith was the player's pet
  • lua files and entries in the new pet_skills.sql table created for all blood pacts:
    • Regal Slash (BP:Rage): 3-hit physical
    • Level ? Holy (BP:Rage): aoe magical
      • Rolls a die and does dmg proportional to roll
      • Only does damage if the target's level is divisible by the roll
    • Mewing Lullaby (BP:Ward): AoE lullaby that resets TP on non-NM mobs
    • Eerie Eye (BP:Ward): conal silence/amnesia with appropriate elemental resist check for amnesia, but retail does light check for silence
    • Reraise II (BP:Ward): single-target 60-minute reraise II buff for any party member
    • Raise II (BP:Ward): single-target raise II for any party member
    • Altana's Favor (BP:Ward): 2-hour ability gives arise to all party members in range (raise 5 and reraise III with infinite duration)
      • couldn't get targetfind to work properly between dead and alive players, so I loop over all party members and raise them manually

Steps to test these changes

This PR does not include the changes from clay's PR, but was tested using the changes

Testing of Level ? Holy is best done on mobs that range around lvl 60 since that level is divisible by all numbers 1-6.

The others are pretty straightforward I think. Giving your pet !immortal makes things easier to test the BP:Rages as well.

@github-actions
Copy link

✨ Thanks for the PR! ✨

This is a friendly automated reminder that the maintainers won't look at your PR until you've properly completed all of the checkboxes in the pre-filled template.

@MowFord
Copy link
Contributor Author

MowFord commented Oct 20, 2023

The crux of my issue with level ? holy is that there seems to be a fundamental flaw in how pet abilities process. The onabilitycheck is executed on the owner's ability, so you can't edit the animation ID of the petskill. (i.e. like I do in this mobskill)

@WinterSolstice8 WinterSolstice8 self-assigned this Oct 20, 2023
@WinterSolstice8
Copy link
Member

I'm busy with other stuff, I wrote the petskill system and there should be a way to iron out the kinks, i'll try to look into this at some point on the weekend. Thanks for your efforts

@MowFord
Copy link
Contributor Author

MowFord commented Oct 20, 2023

I'm busy with other stuff, I wrote the petskill system and there should be a way to iron out the kinks, i'll try to look into this at some point on the weekend. Thanks for your efforts

Sounds good. I'm going to work on cleaning up the lua I just wanted to get something in front of you so I'm not completely in the dark.

Also just to be clear: this code works on a technical level but

  • I'm not a fan of hard coding the animation changes in cpp
  • the messages don't work perfectly as it seems the ability id is referencing mob skills instead of pet skills (i.e. if we set reraise_ii to use message id 317, it says cait sith used berserk, because berserk is mob skill id 526

@WinterSolstice8
Copy link
Member

Battlemod doesn't seem to like Cait Sith (LSB image)
image
testing retail now...

@WinterSolstice8
Copy link
Member

Cait Sith's name is wrong (missing the space) and the pool/group has "The" flagged, retail msg:

image

Level ? Holy needs to have the primary message set to skill:setMsg(xi.msg.basic.JA_NO_EFFECT_2) if it misses per a retail cap

image

@WinterSolstice8
Copy link
Member

Reraise II message is JA_GAIN_EFFECT_2 = 316, -- <user> uses <ability>. <target> gains the effect of <effect>.
(previously undocumented in msg.lua)

@WinterSolstice8
Copy link
Member

I see how to fix the problem with level ? holy, because the animation (presumably) stores the "roll" for what the level ? holy will do damage to, and rerolling it is not retail behavior.

I think I just need to add an accessor to get the action's first target and from that, you'll be able to get the action's first animation and/or set it appropriately. I'll hopefully get a PR tomorrow for the change.

@WinterSolstice8
Copy link
Member

Level ? holy script works with some backend changes like this:

abilityObject.onPetAbility = function(target, pet, skill, master, action)
    local holyRollOneAnimID = 164
    local primaryTargetID = action:getPrimaryTargetID()

    -- If primary target, roll for power by setting random animation.
    if primaryTargetID == target:getID() then
        action:setAnimation(primaryTargetID, holyRollOneAnimID + math.random(0, 5))
    else
        action:setAnimation(target:getID(), action:getAnimation(primaryTargetID))
    end

    local power = action:getAnimation(target:getID()) - holyRollOneAnimID + 1
    local dMND = math.floor(pet:getStat(xi.mod.MND) - target:getStat(xi.mod.MND))
    local ele = xi.damageType.LIGHT
    local dmg = 0

    local dmgmod = 1
    local basedmg = pet:getMainLvl() * power + (dMND * 1.5)

@WinterSolstice8 WinterSolstice8 mentioned this pull request Oct 22, 2023
4 tasks
@MowFord
Copy link
Contributor Author

MowFord commented Oct 22, 2023

Apologies if it fails sanity checks, have to step away before doing that step, but bps are tested to work

@MowFord
Copy link
Contributor Author

MowFord commented Oct 22, 2023

Unfortunately the issue i found with altana's favor is also happening for raise ii. I thought i had confirmed it works but I cannot get the ability to consider a dead target valid for the ability :(

I diagnosed a good bit and best I can tell the issue is related to CCharEntity::ValidTarget. I set the flag 32 for target_player_dead but it just doesn't even call the onAbilityCheck function

@MowFord
Copy link
Contributor Author

MowFord commented Oct 28, 2023

If it's simpler to get things moving we could disable Raise II in abilities.sql to get Cait Sith implemented the rest of the way for now. I know you'd rather not have half-implemented thing, but Cait Sith being unimplemented at all might be a special case?

@WinterSolstice8
Copy link
Member

might be able to look at it later tonight, stay tuned

@WinterSolstice8
Copy link
Member

image
soon

@WinterSolstice8
Copy link
Member

image

@WinterSolstice8
Copy link
Member

Is there anything else you're missing? Work has been a bit busy.

@MowFord
Copy link
Contributor Author

MowFord commented Nov 5, 2023

Is there anything else you're missing? Work has been a bit busy.

That was everything needed for this PR. I appreciate you. I'll clean up my PR to utilize your changes later this week

Edit: Did a rebase and tested with the changes. All working

@MowFord MowFord changed the title Draft: Cait sith avatar [Avatar] Implement Cait Sith Nov 6, 2023
@Xaver-DaRed Xaver-DaRed added enhancement New feature request and removed enhancement New feature request labels Nov 6, 2023
@@ -0,0 +1,30 @@
-----------------------------------
-- Regal Scratch
-- M=3,1
Copy link
Member

Choose a reason for hiding this comment

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

what's this M=3,1? doesn't really bother me that it's in there i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha good question, was a direct copy from what we had on wings... i honestly have no idea

- Cait sith has proper name prefix and named properly to be "Cait Sith" instead of "The CaitSith"
- BPs Implemented
  - Regal Slash (BP:Rage): 3-hit physical
  - Level ? Holy (BP:Rage): aoe magical
    - Rolls a die and does dmg proportional to roll
    - Only does damage if the target's level is divisible by the roll
  - Mewing Lullaby (BP:Ward):
  AoE lullaby that resets TP
  - Eerie Eye (BP:Ward):
  conal silence/amnesia with appropriate elemental resist check for amnesia, but retail does light check for silence
  - Reraise II (BP:Ward):
  single-target 60-minute reraise II buff for any party member
  - Raise II (BP:Ward):
  single-target raise II for any party member
  - Altana's Favor (BP:Ward):
  2-hour ability gives arise to all party members in range
  (Arise and reraise III with infinite duration)
@Xaver-DaRed Xaver-DaRed added enhancement New feature request and removed enhancement New feature request labels Nov 8, 2023
@TeoTwawki TeoTwawki merged commit e07568f into LandSandBoat:base Nov 12, 2023
13 checks passed
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.

4 participants