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

SAM Iaijutsu/Tsubame on Shinten/Kyuten #378

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aldros-ffxi
Copy link

Ok, so...

I've been wanting to do something like this, since I like to have all my main stuff on the face buttons...

However, I run into a weird issue, replacing Iaijutsu with Tsubame-gaeshi only works with 2 sen, not 3 for some reason.

@aldros-ffxi aldros-ffxi marked this pull request as draft August 5, 2024 18:50
@aldros-ffxi
Copy link
Author

Currently, how I've gotten it working is setting iaijutsu to the AoE variants, and single targets to Tsubame. In this case, it doesn't bring up Tsubame until none of the other conditions pass, but it's usable.

At a slight loss for how to proceed beyond that, it wouldn't be functional for actual use, but works fine for my testing

@aldros-ffxi
Copy link
Author

Actually, now that I think of it, I could just put Iaijutsu/Tsubame on Shinten/Kyuten

@aldros-ffxi aldros-ffxi changed the title SAM Kenki consumers on Iaijutsu/Tsubame SAM Iaijutsu/Tsubame on Shinten/Kyuten Aug 5, 2024
@aldros-ffxi aldros-ffxi force-pushed the feature/SAM-button-condense branch 2 times, most recently from b1d717e to cd6ef35 Compare August 5, 2024 23:17
@MKhayle
Copy link
Owner

MKhayle commented Aug 5, 2024

I don't know if you're aware of it, but there is currently an ongoing rework of the plugin in #323 that may interest you (in https://github.com/MKhayle/XIVComboExpanded/tree/AttributesRework)

@kaedys
Copy link

kaedys commented Aug 5, 2024

FYI, I'm revising the existing Iaijutsu -> Tsubame feature to use the new CanUseAction instead of a Sen check, and then just straight up deleting the Tsubame->Iaijutsu version, since Tsubame doesn't have a cooldown anymore anyway, so there's no need to track it separately on your bar like there used to be. CanUseAction in particular works extremely well for it:

                var original = OriginalHook(SAM.TsubameGaeshi);
                if (level >= SAM.Levels.TsubameGaeshi && CanUseAction(original))
                    return original;

@aldros-ffxi
Copy link
Author

Yeah, I know I need to go read over that change set eventually and port my changes, but I just haven't had the energy xD

And yeah, I replaced the Sen check with !IsOriginal since it's more reliable

@kaedys
Copy link

kaedys commented Aug 5, 2024

I'm not sure on putting Shinten/Kyuten on Iaijutsu, though. Wouldn't that only let you use Iai/Tsu if you're low on Kenki? Or if that's reversed, only let you use Shinten/Kyuten if you cannot use Iai/Tsu? It's worth noting that the currently posted opener for SAM does weave between Iaijutsu and Tsubame. I don't believe that's yet been updated for the change to Tendo's GCD, but given the 1.3s cast and 2.5s, it's absolutely feasible to single-weave between the two without clipping (no different than single-weaving after a healer nuke, actually), and I just verified that that on a dummy.

Are the existing features that merge Kyuten, Shoha, and Senei insufficient? I would think even on controller, though, having one set of 4 buttons dedicated to the 3 combos and Iaijutsu/Tsubame, and then the other 4-button set dedicated to Kenki and other crap would work pretty well, with the alt crossbars handling all of your less common stuff.

@aldros-ffxi
Copy link
Author

The way I had it written (which I just noticed an issue) was to have it only usable when Iai/Tsu weren't, which is how I'd been using it mainly is dumping all Kenki right after Iai/Tsu

@kaedys
Copy link

kaedys commented Aug 5, 2024

I would think that would also prevent dumping Kenki at any point between gaining your first Sen and spending them on Midare.

@aldros-ffxi
Copy link
Author

And yeah, the way I use controller may be a little weird, I tend to camp one side for single target and the other for AoE, putting CDs on the arrows and main abilities on face buttons, stuff I rarely use on the hold both triggers bar

@kaedys
Copy link

kaedys commented Aug 5, 2024

I would tend to argue that Iaijutsu/Tsubame is the one that should be yeeted to the trigger bar, then, since you use it roughly every ~20 seconds, at least for a Midare. I just think you're going to find it unnecessarily limiting and problematic trying to combine your Kenki spenders and Iaijutsu.

@aldros-ffxi
Copy link
Author

Yeah, I can always just carry it on my local if necessary

@kaedys
Copy link

kaedys commented Aug 5, 2024

I haven't even begun rebasing my local branch to the rework yet >.< I'll wait until it's more finalized. Though at least 3/4ths of the changes I have locally, I'll be PRing at some point as well.

@aldros-ffxi
Copy link
Author

That gave me a great idea though, if I check cooldown, it'll let it weave oGCDs

@kaedys
Copy link

kaedys commented Aug 6, 2024

Yep, I'm using exactly that behavior to better handle the Sonic Break + Bow Shock feature for Gunbreaker. It uses Sonic Shock if it's "off cooldown" (ie. GCD not active), and Bow Shock otherwise. It works way better than I expected, too. May have to (ab)use that in other areas.

@aldros-ffxi
Copy link
Author

So I just tested it, and this actually works perfectly, it'll only allow you to use Iai/Tsu when the GCD is up, and weave otherwise

@aldros-ffxi
Copy link
Author

I think the only potential drawback is the inability to queue Iai/Tsu since you can't hit the button until it's up

@aldros-ffxi
Copy link
Author

Hmmm, it's a little finnicky, it looks like it might actually be queueing it, but it's just slow to visually display the icon

@MKhayle MKhayle self-requested a review August 10, 2024 01:11
@MKhayle
Copy link
Owner

MKhayle commented Aug 12, 2024

is it still a draft?

@aldros-ffxi
Copy link
Author

So... It should be good, but for some reason it causes a crash on load now...

Other than that, it works but the interaction with the GCD is a little odd, it might need to cook a bit more

[SectionCombo("Kyuten")]
[ExpandedCustomCombo]
[CustomComboInfo("Shinten to Iaijutsu", "Replace Hissatsu: Shinten with Iaijutsu when available.", SAM.JobID)]
SamuraiShintenIaijutsuFeature = 3422,
Copy link

@kaedys kaedys Aug 13, 2024

Choose a reason for hiding this comment

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

This is why it is crashing, this combo ID is now in use by SamuraiIaijutsuSingleSenNoReplaceTsubameFeature.

Edit: this is actually the only way I've ever gotten this mod to fail to load, incidentally. So if the mod fails to load, that is the first thing to check by a mile. Also, the Dalamud logs do show that. It'll look something like:

2024-08-13 11:18:07.640 -04:00 [ERR] [LOCALPLUGIN] Exception during plugin initialization
System.AggregateException: Failed to create XIVComboExpandedPlugin.XIVComboExpandedPlugin (ctor invocation) (Exception has been thrown by the target of an invocation.)
 ---> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.ArgumentException: An item with the same key has already been added. Key: FeatureThatHasADuplicateID

@aldros-ffxi
Copy link
Author

Oh dang, that's not caught at compile time anymore D:

@aldros-ffxi aldros-ffxi marked this pull request as ready for review August 13, 2024 22:22
@MKhayle
Copy link
Owner

MKhayle commented Aug 14, 2024

Duplicate array keys are, values aren't

@aldros-ffxi
Copy link
Author

aldros-ffxi commented Aug 18, 2024

Getting back to this, I think there's some other optimizations potentially using the CD of the oGCDs too

Is there a way to have it predict the CD (like get the exact cd amount rather than just is off cooldown)

@kaedys
Copy link

kaedys commented Aug 18, 2024

For abilities that have cooldowns, yes. So for example, Shinten has a 1.0s cooldown, as most spammable oGCDs have, and that can be checked using GetCooldown(action), then checking either the CooldownRemaining or TotalCooldownRemaining fields (the difference is the former is time until next usable, the latter is time until at maximum charges. They're equivalent for non-charge-based abilities).

However, that's the cooldown for back-to-back cast, not the animation lock. We do not currently have any mechanism for checking the animation lock. I know it can be done, because Clippy is a thing, but we'd have to dig into how Clippy interacts with the animation lock system to check (and to hopefully not trample on it or XIV-A's trimming of the animation lock).

It's also worth noting that due to the changes in 7.05, I'd argue the feature in this one is possibly even worse, since Tsubame can now be held for up to 30 seconds after Midare/Goken, and is intentionally held when going into each of the 60s burst windows. Basically, you save the Tsubame from the Midare halfway between the bursts phases, then go into the burst phase with both a Tsubame and 3 Sen, so you can do Tsubame -> Meikyo -> Tendo -> Tendo Tsubame -> use Meikyo to apply Hig and go into another Midare.

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.

3 participants