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

[Item] Add all phase 4 trinkets including item notices #1059

Merged
merged 13 commits into from
Oct 2, 2024

Conversation

hillerstorm
Copy link
Contributor

@hillerstorm hillerstorm commented Sep 26, 2024

Also changed Spidersilk Spindle to reduce incoming damage (like Savage Defense) instead of healing to be able to see its effects on incoming damage.
All proc masks and coefficients should be correct to how it is in the dbc and reading up on patch changes.

Added item notices for all of the procc trinkets, including the healing ones not being implemented.
Also added "not ingame" notices for the 384/410 variants of the Valor Trinkets which, just like some Firelands stuff, were never actually in game.

@1337LutZ
Copy link
Contributor

@hillerstorm I asked @RobFalfa to take a look at this and compare it with his implementations

@zoristaken
Copy link
Contributor

About adding notices to items that were never in the game, I think It should be more correct, the moment we have confirmation that those versions won't exist in classic, to blacklist them instead of having them available as a choice. Seems fine for now since we don't know yet if they will reuse them.

@hillerstorm
Copy link
Contributor Author

About adding notices to items that were never in the game, I think It should be more correct, the moment we have confirmation that those versions won't exist in classic, to blacklist them instead of having them available as a choice. Seems fine for now since we don't know yet if they will reuse them.

Yea that's my thinking as well. As soon as we get confirmation of blizzard not adding the upgraded firelands items we should blacklist them etc.
The text could perhaps be even more detailed, explaining a bit more.

@1337LutZ
Copy link
Contributor

About adding notices to items that were never in the game, I think It should be more correct, the moment we have confirmation that those versions won't exist in classic, to blacklist them instead of having them available as a choice. Seems fine for now since we don't know yet if they will reuse them.

Yea that's my thinking as well. As soon as we get confirmation of blizzard not adding the upgraded firelands items we should blacklist them etc. The text could perhaps be even more detailed, explaining a bit more.

Yea I agree.

sim/common/cata/other_effects.go Outdated Show resolved Hide resolved
sim/common/cata/other_effects.go Outdated Show resolved Hide resolved
sim/common/cata/other_effects.go Outdated Show resolved Hide resolved
sim/common/cata/stat_bonus_stacking.go Outdated Show resolved Hide resolved
@hillerstorm
Copy link
Contributor Author

Updated the item-doesnt-exist-notice a bit:
image

@1337LutZ
Copy link
Contributor

Updated the item-doesnt-exist-notice a bit: image

Nice!

@hillerstorm
Copy link
Contributor Author

I added a TODO comment on Vial of Shadows regarding the AP modifier with some links for reference later.

Would you feel comfortable with merging this @NerdEgghead seeing as I touched Savage Defense? :) that change didn't result in any test diffs and from my own testing it looked great.

@NerdEgghead NerdEgghead merged commit bad44b8 into wowsims:master Oct 2, 2024
2 checks passed
@hillerstorm hillerstorm deleted the ds_trinkets branch October 2, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants