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

Fix: Unable to set spell group to 'NONE' #4872

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Tofame
Copy link
Contributor

@Tofame Tofame commented Dec 10, 2024

Pull Request Prelude

Changes Proposed

Using the spellgroup("none"), didn't work as below in the code, after it was already loaded, the code: if (group == SPELLGROUP_NONE) {, would have overriden it and set none to either attack or healing group.

This successfully fixes it.

If group is none, then we don't want to load/change group cooldowns, as other checks in the code for spell cooldown check only groupCooldown > 0, and not group != SPELLGROUP_NONE, so I think it will be more efficient to block it at loading.

Issues addressed:
'none' spell tag not working.
How to test:
Try to set spell's group to none, you will see using the spell will still put cd on attacking/healing group as this is what none is set to currently.

Using the spellgroup("none"), didn't work as below in the code, after it was already loaded, the code: if (group == SPELLGROUP_NONE) {, would have overriden it and set none to either attack or healing group.

This successfully fixes it.

If group is none, then we don't want to load/change group cooldowns, as other checks in the code for spell cooldown check only groupCooldown > 0, and not group != SPELLGROUP_NONE, so I think it will be more efficient to block it at loading.
nekiro
nekiro previously approved these changes Dec 10, 2024
@Tofame Tofame marked this pull request as draft December 10, 2024 10:18
@Tofame
Copy link
Contributor Author

Tofame commented Dec 10, 2024

Need to consider that initialization of variable groupCooldown is 1000, which is not supported by ifs related to applying/checking cooldown.
Need to further consider revscripts, as I changed XML parsing, but apparently revscript loading would need changes too.

@Tofame
Copy link
Contributor Author

Tofame commented Dec 10, 2024

Okay so as @ArturKnopik said when I asked him on discord, he told me every spell has a group

So I've read more into that and currently he is right. There are 2 options that I think are viable:

  1. Removing any code that takes parameters 'none'/'0' regarding to group in XML parser as its misleading, and not do anything more, leave SPELLGROUP_NONE as serving as 'undefined' group, then we assume that every spell is forced to have some kind of group. [This is how it works in TFS now]
  2. Allow for spells to actually not have groups, so they can only have their own cd and not impose cooldowns on groups. This would require changes that I already submitted and a little change in revscript spell group loading. [This is what I suggest]

What do you think?

@Tofame
Copy link
Contributor Author

Tofame commented Dec 10, 2024

Updated commit to adapt revscript loading, showing how it could look.

I dont like how I had to add new enum, especially with value 255, but Im pretty restricted as I must have in mind to keep everything compatible.

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