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

Summoner Ruin/Outburst Combos #263

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

Conversation

Binarynova
Copy link

@Binarynova Binarynova commented Jan 18, 2023

Per Tansoric's suggestion, this PR adds a new combo option for Summoner.

This makes:

  • Ruin change to Topaz/Ruby/Emerald Ruin I/II/III (as appropriate by level)
  • Outburst change to Topaz/Ruby/Emerald Outburst (before level 74)
  • Outburst change to Topaz/Ruby/Emerald Disaster (starting at level 74)

A great perk of this, is that it makes Ruin act similarly for Ifrit/Garuda/Titan to the way it already does with Bahamut/Phoenix.

This took a bit more doing than I expected. Trying to simply replace Ruin with Gemshine resulted in Gemshine not changing to the attuned avatar's Ruin variant (similar issues with Tri-Disaster and Precious Brilliance). So I manually handle the level logic for all the swapping. Someone with more understanding of the base code might be able to do this more cleanly.

I've used this one quite a bit and think it might be ready to be merged. I haven't done a lot of pull requests before, so I'm open to conversations about process if I did something wrong. Thanks!

This PR adds a new combo option for Summoner.

This will replace Ruin with the appropriate Topaz/Ruby/Emerald Ruin and will replace Outburst with the appropriate Topaz/Ruby/Emerald Outburst or Disaster, as appropriate for your level and current Avatar attunement.
Forgot to add "Catastrophe" Spells starting at level 82.
// Change Ruin/Outburst into Topaz/Emerald/Ruby Ruin/Outburst
if (Configuration.ComboPresets.HasFlag(CustomComboPreset.SummonerRuinOutburstCombo))
{
if (actionID == SMN.Ruin1)
Copy link

@RaphaelDDL RaphaelDDL Jun 1, 2023

Choose a reason for hiding this comment

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

I liked this change so while isn't merged or anything, I checked out the PR, built and tested in lv90 SMN. Didn't test on lower levels yet.
The outburst part worked, even by dragging tri-disaster (the upgrade) from actions.

Now, SMN ruin goes 1, 2, 3 so testing for ruin1 as you added should've worked I guess? But I'm confused, it didn't for a lv90 SMN which has Ruin3 and at level 90, you can only drag Ruin3 from actions.

I did a quick change on the debug build to test for SMN to if (actionID == SMN.Ruin3 || actionID == SMN.Ruin1) and now it replaces Ruin3 correctly /shrug

I have no idea where to get the actionID for Ruin2 in order to add it to JobActions/SMN.cs and then add actionID == SMN.Ruin2 to the if and see if works.

I still gotta test lower levels but I hope @attickdoor does merge this (after the fixes) as it checks out all 3 rules of #119 : The actions, while can be cast during an egi summon phase, are in fact mutually exclusive (when during summon, there's no reason to go for tri and ruin), they save 2 slots to enabling other actions more nearby and is a 0 IQ non-whacky change xD

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.

2 participants