-
Notifications
You must be signed in to change notification settings - Fork 260
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: ally-only spells no longer autotarget enemies, fix crash when casting SWAP_POS
on a tile with no creature to swap with
#5338
fix: ally-only spells no longer autotarget enemies, fix crash when casting SWAP_POS
on a tile with no creature to swap with
#5338
Conversation
…sting `SWAP_POS` on a tile with no creature to swap with
Autofix has formatted code style violation in this PR. I edit commits locally (e.g: git, github desktop) and want to keep autofix
I do not want the automated commit
If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple enough, I'm going to be testing it later.
One thing though: maybe I should also make it so SWAP_POS also doesn't teleport the user if the destination is impassible? |
I don't know about that. I think that's bad for a few reasons, assuming you can target an unknown area:
|
That's what I mean, I'm asking if I should make it also check whether you're going to get yourself stuck in a wall and if so don't move the caster. |
I meant that fixing it would make it more annoying to get a valid spot to teleport to without clairvoyance and that you could still use it to fish for where walls are. Getting stuck in a wall doesn't generally kill players as I do it with debug and it doesn't seem to cause damage, but maybe there's something in the code I don't know about. Worst case is you teleport back out. |
Ah, fair enough then. Letting the player teleport themselves into walls is easily fixed by just teleporting back out again, or walking out if a valid space is nearby. |
Checklist
Required
main
so it won't cause conflict when updatingmain
branch later.Optional
Purpose of change
This fixes a couple long-running annoyances in spellcasting code I figured out solutions for. Also technically opens up unrestricted controlled teleportation as something spells can do without even needing a new spell function.
Fixes #4216
Describe the solution
C++ changes:
swap_pos
so that it only tries to move any creatures found in the epicenter if there actually is one. If the tile targeted is empty, now instead of the game just imploding like before, it simply warps you over there without any complaints, meaning you can freely useSWAP_POS
spells on any tile you want if you write the spell's JSON to permit targeting the ground to just use it for teleportation if you so choose.target_ui::choose_initial_target
so that spells don't try to autotarget any enemy in range unless they can ACTUALLY be used on hostiles, so you won't be prompted to try and waste your time casting a spell only usable on allies, and likewise making it less annoying trying to heal or buff yourself in the middle of a fight via whatever's kicking your ass trying to run off with your cursor.Documentation changes:
SWAP_POS
does in magic.md.Describe alternatives you've considered
Adding a spell in Magiclysm that uses
SWAP_POS
as a full-on blink spell. Robbie and Fox said they have plans already so will leave it to them.Testing
Casual bullying of an NPC with a version of Holographic Transposition temporarily given damage, and it working without issue when the victim dies from it:
Additional context
Checked in DDA, as of build
2024-08-30-1256
targeting an invalid tile with aSWAP_POS
spell still crashes the game:Even better, a basic attempt to test and confirm if you still auto-target non-allies with Windrunning revealed that the spell I selected on the spellcasting menu ends up not being the spell it tries to cast:
So I have to try and cast Shocking Lash instead, which ACTUALLY casts Windrunning, and only then can I confirm DDA auto-targets non-allies with ally-only spells.