Skip to content

threnody can be overwritten by same tier - #11529

Open
caerulius wants to merge 1 commit into
LandSandBoat:basefrom
caerulius:threnody_self_overwrite
Open

caerulius wants to merge 1 commit into
LandSandBoat:basefrom
caerulius:threnody_self_overwrite

Conversation

@caerulius

Copy link
Copy Markdown
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?

Related to this issue.

This change makes it so threnodies can be overwritten by threnodies of the same tier or higher, but not by ones of a lower tier.

It's not the cleanest implementation but it is the simplest: The function that now has the exception for threnody causes this not to work because each of the threnodies has the same internal effect id. When the code runs it sees an effect with the same id at the same tier, and short circuits it to "no effect". Skipping this check for threnody specifically lets the normal tier decisioning happen.

This is all trusting that bug report and hinging on whether or not this is retail behavior. I couldn't find an example of someone swapping a threnody, so if someone has a bard and can test it that'd be good. If you cant turn fire threnody i into ice threnody i on a mob in retail, then this PR should just be closed.

Steps to test these changes

Monster with fire threnody I on it -> use ice threnody I, it gains it -> use fire threnody ii, it gains it -> use ice threnody i, no effect -> use ice threnody ii, it gains it.

@siknoz

siknoz commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

retail: so yes, thren can overwrite thren, thren 2 can overwrite thren 1 and thren 2, but thren 1 cannot overwrite thren 2

image

Comment thread scripts/globals/spells/enfeebling_song.lua
@caerulius
caerulius force-pushed the threnody_self_overwrite branch from 611f83d to 56926e8 Compare September 18, 2026 05:33
@Xaver-DaRed Xaver-DaRed added the hold On hold, pending further action/info label Sep 18, 2026
@caerulius

Copy link
Copy Markdown
Contributor Author

For this PR how would you like me to finish it? The suggested formatting is rejected by the sanity_checks. I can revert to the formatting I had before or we can override the sanity checks.

@zach2good

Copy link
Copy Markdown
Contributor

or we can override the sanity checks.

For reference, we don't even make this allowance for LSB core contributors. The answer is to keep pushing until CI passes (the CI minutes are free!)

@zach2good

Copy link
Copy Markdown
Contributor

@caerulius

Copy link
Copy Markdown
Contributor Author

@caerulius
caerulius force-pushed the threnody_self_overwrite branch 3 times, most recently from 3d09208 to 8893e7f Compare September 18, 2026 15:50
@caerulius

Copy link
Copy Markdown
Contributor Author

Sanity checks and data formatting passed, just waiting on the builds. Do you need anything more from me to drop the hold tag?

@caerulius
caerulius force-pushed the threnody_self_overwrite branch from 8893e7f to 8896db0 Compare September 18, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hold On hold, pending further action/info

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants