Skip to content

Add six new item mod IDs#7516

Merged
Xaver-DaRed merged 1 commit intoLandSandBoat:basefrom
wccbuck:new_item_mod_ids
May 2, 2025
Merged

Add six new item mod IDs#7516
Xaver-DaRed merged 1 commit intoLandSandBoat:basefrom
wccbuck:new_item_mod_ids

Conversation

@wccbuck
Copy link
Copy Markdown
Contributor

@wccbuck wccbuck commented May 1, 2025

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?

This PR adds, but does not yet implement, the following item IDs, and assigns them to appropriate gear:

  • ENHANCES_FUTAE (e.g. Hattori Tekko)
  • ENHANCES_ELEMENTAL_SEAL (e.g. Laevateinn)
  • ELEMENTAL_DEBUFF_EFFECT (e.g. Agwu's Slops)
  • ENF_MAG_DURATION (e.g. Regal Cuffs)
  • REWARD_RECAST (e.g. Khimaira Bonnet)
  • AUGMENTS_ABSORB_TP (e.g. Bale Gauntlets +1)

In addition, this PR fixes two bugs I noticed while implementing these changes. Namely:

  • NIN_NUKE_BONUS_GEAR (Ninjutsu MAB) is now appropriately applied to ninja spells (it was using the wrong mod before)
  • One of the versions of Laevateinn had "quick draw dmg" instead of buffing Vidohunir damage, not sure how that happened

Steps to test these changes

The new item mods aren't hooked up yet so no real way to test those, but you can confirm that ninja MAB gear is now working by performing a ninja nuke with and without Koga Hatsuburi equipped.

Comment thread src/map/modifier.h Outdated
@wccbuck wccbuck force-pushed the new_item_mod_ids branch from 237e5f4 to aa6b877 Compare May 1, 2025 10:46
@Xaver-DaRed
Copy link
Copy Markdown
Contributor

We don't usually allow adding random modifier IDs without them being actually implemented.
The main reason why is because the chances of them being forgotten and then being implemented down the line resulting in a duplicate is quite high.

I would propose this new, non-implemented, modifiers to be all placed at the end of the list, under a comment that states This modifier isnt implemented so they are all together and so we can easily track them, in case some1 decides to implement them in the future.

@wccbuck
Copy link
Copy Markdown
Contributor Author

wccbuck commented May 1, 2025

We don't usually allow adding random modifier IDs without them being actually implemented. The main reason why is because the chances of them being forgotten and then being implemented down the line resulting in a duplicate is quite high.

I would propose this new, non-implemented, modifiers to be all placed at the end of the list, under a comment that states This modifier isnt implemented so they are all together and so we can easily track them, in case some1 decides to implement them in the future.

That's fair. To give context, I am intending on adding functionality to all of these item mods over the next week, and decided to make this PR first in order to prevent a mod ID conflict which has happened before. Would it be sufficient for me to replace my "// TODO" with "// NOT YET IMPLEMENTED" on each of these?

@Xaver-DaRed
Copy link
Copy Markdown
Contributor

Xaver-DaRed commented May 1, 2025

-- TODO: this mods are not implemented.
MOD_NAME_1 = n1
MOD_NAME_2 = n2
...

Then once you (or whoever else) decide to implement any of them, they can be moved to a better place.

Just a matter of organizing stuff properly, to make it clear not for a select few, but for as many ppl as possible within reason

@wccbuck wccbuck force-pushed the new_item_mod_ids branch 2 times, most recently from 8c0699c to daaa887 Compare May 1, 2025 20:06
@wccbuck wccbuck force-pushed the new_item_mod_ids branch from daaa887 to dbfce02 Compare May 1, 2025 20:17
@Xaver-DaRed Xaver-DaRed merged commit bd76b5e into LandSandBoat:base May 2, 2025
13 checks passed
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