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

Conditionally prevent damaging and spell auto-targeting Golem #7010

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

kphoenix137
Copy link
Collaborator

@kphoenix137 kphoenix137 commented Mar 7, 2024

Alternative approach to the concept in #6881.
Fixes: #4352

When can a Golem be damaged?

image

By default, you are able to damage any Golem regardless of any settings or damage method.

image

In this PR, you are completely unable to damage your own Golem via any method or setting. For damaging Golems belonging to other players, it mirrors the same behavior as damaging other players, regardless of method or setting.

image

In the linked PR, there is no differentiation between owned/unowned Golems. Other than that, damaging Golems is completely consistent with damaging players based on options/settings.

The idea in this PR is to unify behavior between the ability to damage other players and other players' Golems, and to prevent damage to a player's own golem as a QoL improvement.

Regarding auto-targeting spells

I've revised the behavior of Chain Lightning, Bone Spirit, and Elemental to address the problem originally addressed in the issue before it was renamed. These spells now behave cohesively with the logic for damage in this PR, no longer targeting a Golem if it is unable to damage it. I left some comments for a potential future PR for if we should be preventing auto-targeting spells from targeting monsters they are unable to hit (teleporting monsters, immune monsters).

Potential problems

Problem: Unable to destroy own Golem with melee/arrows/spells
Solution: Casting Golem will no longer consume scroll/charge/mana if the spell is simply killing your Golem rather than creating a new one.
More solutions:
image
image
image
image

@kphoenix137 kphoenix137 changed the title Conditionally prevent damaging Golem Conditionally prevent damaging and auto-targeting Golem Mar 7, 2024
@kphoenix137 kphoenix137 changed the title Conditionally prevent damaging and auto-targeting Golem Conditionally prevent damaging and spell auto-targeting Golem Mar 7, 2024
@kphoenix137 kphoenix137 added the fix fix for a bug label Mar 7, 2024
@ikonomov
Copy link
Contributor

ikonomov commented Mar 7, 2024

Yours makes more sense if we assume golems are like familiars, mine makes more sense if we assume golems are friendly summons. Whichever one works better for AJenbo, either one works for me.

@kphoenix137 kphoenix137 requested a review from glebm March 7, 2024 23:30
@kphoenix137
Copy link
Collaborator Author

kphoenix137 commented Mar 7, 2024

Yours makes more sense if we assume golems are like familiars, mine makes more sense if we assume golems are friendly summons. Whichever one works better for AJenbo, either one works for me.

Yours is viable. What I would change though is the usage of Friendly Fire for melee handling, as the logic is intended for spells/arrows, and not melee, and instead, use player.friendlyMode in its place. Your PR then should be fully consistent across the board (or across the table I made, I guess :D). It would then be the same as mine other than the fact mine stops all damage to player's own golem no matter what.

ikonomov added a commit to ikonomov/DiabloX that referenced this pull request Mar 10, 2024
@ikonomov
Copy link
Contributor

ikonomov commented Mar 10, 2024

Right you are, changed.

Source/spells.cpp Outdated Show resolved Hide resolved
Source/spells.cpp Outdated Show resolved Hide resolved
@kphoenix137
Copy link
Collaborator Author

@ikonomov updated the PR description to represent your changes.

@kphoenix137 kphoenix137 requested a review from AJenbo March 12, 2024 02:59
@kphoenix137
Copy link
Collaborator Author

kphoenix137 commented Mar 12, 2024

If no damage to own Golem as seen as too heavily a change to gameplay, I think it would still be adequate to mirror ikonomov's PR along with the other additions in this PR.

Additionally, it's worth considering never targeting Golems with spells that auto-target monsters. It's clear that in the original code, Guardians will never attack a Golem on purpose no matter what. Perhaps the same logic should be adopted for Chain Lightning, Elemental, and Bone Spirit?

@kphoenix137 kphoenix137 mentioned this pull request Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Player's spells damage player's golem
3 participants