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

[SMN] Add option to replace Emerald Ruin III with Ruin III #121

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

Conversation

GayPotatoEmma
Copy link

Figured I'd try my hand at implementing #41 myself, seems to work fine. Any feedback is appreciated.

@GayPotatoEmma GayPotatoEmma changed the title Add option to replace Emerald Ruin III with Ruin III [SMN] Add option to replace Emerald Ruin III with Ruin III Nov 25, 2024
@zbee zbee added the New Feature This adds a new option or feature label Nov 25, 2024
Copy link
Member

@zbee zbee left a comment

Choose a reason for hiding this comment

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

Would like to see the restrictions on level and GCD actually put into the code.

WrathCombo/Combos/CustomComboPreset.cs Outdated Show resolved Hide resolved
@GayPotatoEmma
Copy link
Author

GayPotatoEmma commented Nov 26, 2024

Would like to see the restrictions on level and GCD actually put into the code.

I'll have to see if I can figure out how that is done for the GCD, for the level itself it won't actually matter anymore since I've made it so it doesn't override Emerald Rite (the upgraded version from level 72 onwards).

Edit: Just ran some calculations, even the GCD note might be redundant infact as Ruin III would need a +/- 3s GCD to be worse than Emerald Ruin III. So I may remove that note completely

Copy link
Contributor

@Genesis-Nova Genesis-Nova left a comment

Choose a reason for hiding this comment

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

The code for it looks fine, but I'd move it down to right above gemshine's place, which is line 733. the // Gemshine line

Simply moved the code for this down in priority next to the normal gemshine code to ensure it doesn't take priority over more important things (ogcds, etc)
Copy link
Contributor

@Taurenkey Taurenkey left a comment

Choose a reason for hiding this comment

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

This will require a small rewrite.

  • Remove checking what Gemshine is, favour checking the gauge for how much attunement is left instead (you're already checking if Garuda attuned so this won't be an issue for other attunements)
  • Add a trait check for Ruin Mastery III (ID 476) to ensure it's used before Emerald Rite since at that point it becomes a DPS loss.
  • Rewrite the feature description to outline this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature This adds a new option or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants