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 spells interaction with walls #3493

Merged
merged 6 commits into from Aug 9, 2021
Merged

fix spells interaction with walls #3493

merged 6 commits into from Aug 9, 2021

Conversation

Zbizu
Copy link
Contributor

@Zbizu Zbizu commented Jul 1, 2021

Pull Request Prelude

Changes Proposed

  • remove the check for toPos for instant spells
  • move the centerPos for isSightClear checks to avoid casting waves through walls

Issues addressed: #2891

Note: Not tested yet, side effects unknown.

remove the check for tile in front of player for area spells
attempt to fix #2891
@EPuncker EPuncker requested a review from a team July 1, 2021 13:53
@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 1, 2021

tested, doesn't fix the issue for clarity: already fixed

the problem is that check for instant spells calls canDoCombat

ReturnValue ret = Combat::canDoCombat(player, tile, aggressive);

which checks for blockprojectile

if (tile->hasProperty(CONST_PROP_BLOCKPROJECTILE)) {

but in order to remove it, I need to make sure that onAreaCombat gets executed somehow

edit: gets called later, can safely be removed

@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 1, 2021

removing the check for possibility to cast the spell made it also possible to cast the spell through wall because isSightClear was checking the tile in front of the player instead of the player himself so I fixed that as well

spells such as ue work normally, just tested

edit: breaks diagonal wall spells, needs some fixing
edit2: was already broken, just tested other builds

@Zbizu Zbizu marked this pull request as ready for review July 1, 2021 16:25
@MillhioreBT
Copy link
Contributor

MillhioreBT commented Jul 1, 2021

I'm a little concerned about Combat::canDoCombat why should it be removed from here? Doesn't this prevent the spell from doing unnecessary checks?

At first glance it seems a simple solution, but not very pretty

@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 1, 2021

@MillhioreBT canDoCombat is checking the first tile in front of the caster. Removing that check from canDoCombat can bug some other function that uses it. Removing canDoCombat from this place doesn't bug anything as other checks were perfomed earlier and pz check will be performed when checking isAggressive parameter.

@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 1, 2021

Tested my changes on a build from November 2020 now, works fine with diagonal fire/poison/energy wall runes.

@nekiro
Copy link
Member

nekiro commented Jul 1, 2021

What if you throw field rune at stairs (floorchange item) or so, wont it go down/up?

@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 1, 2021

What if you throw field rune at stairs (floorchange item) or so, wont it go down/up?

not enough room error
rune doesn't disappear
rl: fires shooteffect, this pr: doesn't
both send poff

fire bomb doesn't change floors (both servers)

you can aim fire bomb at stairs tile, not sure if this pr allows that, will test tomorrow

@EPuncker EPuncker linked an issue Jul 2, 2021 that may be closed by this pull request
@EPuncker EPuncker added the bugfix label Jul 2, 2021
@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 7, 2021

testing update (rl):
spells can be fired to any place that doesn't block projectile, this includes fences and windows (so blocksolid = true, blockprojectile = false)

field spells launch shoot effect, but return "there is not enough room", poff and don't consume the rune when they can't be placed (eg. when you click on opened window)

area spells can be thrown to any tile that doesn't block projectile, the charge is consumed regardless of spell area

going to look into the code later

@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 8, 2021

Looks like spells and isSightClear are directly tied. This limits my ability to work on them separately. I'll give that a closer look when working with #3487.

@Zbizu Zbizu marked this pull request as draft July 8, 2021 14:57
@Zbizu
Copy link
Contributor Author

Zbizu commented Jul 18, 2021

Delayed until new isSightClear gets merged. Files conflict very likely to happen so I'll rebase this one after isSightClear gets merged.

@Zbizu Zbizu marked this pull request as ready for review August 4, 2021 13:06
@Zbizu
Copy link
Contributor Author

Zbizu commented Aug 4, 2021

new isSightClear was merged, resuming work on that

update: merged commits from main repo
update: wave spells interaction with walls should be fixed now (local tests show me that it works)

src/combat.cpp Outdated Show resolved Hide resolved
@Zbizu
Copy link
Contributor Author

Zbizu commented Aug 8, 2021

Something is stuck in github mergability verification. I addressed the requested changes with a commit already.

@ranisalt
Copy link
Member

ranisalt commented Aug 9, 2021

Something is stuck in github mergability verification

My blessing was required, Github doesn't automatically dismiss change requests

@ranisalt ranisalt merged commit e1e1bc2 into otland:master Aug 9, 2021
@Zbizu Zbizu mentioned this pull request Sep 14, 2021
2 tasks
@Zbizu Zbizu deleted the patch-3 branch September 25, 2021 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Directional spells vs. walls behaviour
5 participants