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

[sql] Add enlight anim to Excalibur additional effect #6187

Merged
merged 1 commit into from
Sep 29, 2024

Conversation

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

Updates the additional effect of Excalibur to light

As shown here

Steps to test these changes

Trigger excalibur additional effect, see the light

@jamesbradleym jamesbradleym changed the title Add enlight anim to Excalibur additional effect [sql] Add enlight anim to Excalibur additional effect Aug 29, 2024
@Xaver-DaRed
Copy link
Contributor

Xaver-DaRed commented Aug 29, 2024

mmm isnt this PR setting excalibur element to light?

Not gonna comment on the animation, that I'm sure its fine, but...

From my understanding...
The issue with excalibur's additional effect, is that it isnt elemental damage, in nature.
One of the reasons Teo wanted "damage type" to be able to be fed into magic functions, was because this additional effect deals "slashing damage"

@Xaver-DaRed Xaver-DaRed added the hold On hold, pending further action/info label Aug 29, 2024
@Xaver-DaRed
Copy link
Contributor

Xaver-DaRed commented Aug 29, 2024

I'll wait on winter to chime in, but, I think this lines:

INSERT INTO `item_mods` VALUES (18276,950,7); -- ITEM_ADDEFFECT_ELEMENT: ELEMENT_LIGHT

Should either:

  • Be deleted and a comment added stating the additional effect needs to be set to "Slashing damage" somehow.
  • Be kept IF they are needed, with a TODO comment stating the element is incorrect and it should be "slashing damage" instead.

@WinterSolstice8
Copy link
Member

I'll wait on winter to chime in, but, I think this lines:

INSERT INTO `item_mods` VALUES (18276,950,7); -- ITEM_ADDEFFECT_ELEMENT: ELEMENT_LIGHT

Should either:

* Be deleted and a comment added stating the additional effect needs to be set to "Slashing damage" somehow.

* Be kept IF they are needed, with a TODO comment stating the element is incorrect and it should be "slashing damage" instead.

Really, excalibur needs its own add effect OR we need to add a ton of parameters to make it possible

excal is an hp% based slashing damage attack but I know that acid bolts are also "piercing damage" but are subject to magic resist checks in certain ways

but yes, probably need to add a TODO here, the PR does solve the problem stated, though it's still not quite accurate. it looks like add effect damage assumes elemental, which is true in most cases

@Xaver-DaRed Xaver-DaRed removed the hold On hold, pending further action/info label Sep 29, 2024
@claywar claywar merged commit bf3d630 into LandSandBoat:base Sep 29, 2024
14 of 15 checks passed
@jamesbradleym jamesbradleym deleted the excalibur-enlight-anim branch September 29, 2024 16:52
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.

4 participants