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

Make Dith hate all fire spells (bepbepimjep, #10969) #590

Merged
merged 1 commit into from
Aug 14, 2017

Conversation

jmbjr
Copy link
Contributor

@jmbjr jmbjr commented Aug 9, 2017

See: https://crawl.develz.org/mantis/view.php?id=10969

Currently, not all fire spells are causing Dith to dish out
penance/wrath.

inner flame
ignite poison
bolt of magma
ignition
firestorm

Had to add some extra BEAMS to apply_beam_conducts() and then point
cast_ignition and cast_fire_storm to the same function.

Ignite_poison is handled similar to how SPELL_SWIFTNESS is handled for
the is_hasty conduct (check for the spell and send a DID_FIRE to
did_god_conduct).

@jmbjr
Copy link
Contributor Author

jmbjr commented Aug 9, 2017

I don't think this is quite ready yet, but wanted to get some comments.
(Thanks to Neil for some late-night help)
edited to update status

DONE: verified that all fire spells now cause Dith to apply penance/wrath
DONE: verified that monsters casting the 5 spells in question don't trigger penance/wrath (so far just checked Cerebov)
DONE: documentation cleanup

DONE: Question: bepbepimjep got credit in the mantis item for reporting this issue. How am I supposed to handle this in my PR? Should I have added (bepbepimjep) in my commit title?

@@ -920,6 +920,10 @@ static void _spellcasting_god_conduct(spell_type spell)
if (is_corpse_violating_spell(spell))
did_god_conduct(DID_CORPSE_VIOLATION, conduct_level);

// need to handle ignite_poison as a DID_FIRE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewing this, I think I should use a comment similar to SPELL_SWIFTNESS below.
// not is_fiery_spell since the other ones handle the conduct themselves

@amalloy
Copy link
Contributor

amalloy commented Aug 9, 2017

Yes, it would be polite to put (bepbepimjep) at the end of the first line of your commit message. That's easy enough for you to fix with git commit --amend, or for anyone to fix when merging this PR.

@jmbjr
Copy link
Contributor Author

jmbjr commented Aug 10, 2017

I can't find any monsters who use Inner Flame, Ignite Poison, and Ignition.
I did confirm that monsters that cast Bolt of Magma do not create any odd penance issues.

Also, I tweaked the comment I mentioned above.

Copy link
Contributor

@amalloy amalloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me stylistically. I don't have any context on the actual logic you've changed, but if you've tested that it makes the intended changes and doesn't break anything obvious seems like a good merge.

If it breaks something non-obvious, I'm sure players will be excited to report it.

@jmbjr jmbjr changed the title Make Dith hate all fire spells Make Dith hate all fire spells (bepbepimjep, #10969) Aug 13, 2017
@amalloy amalloy merged commit 8a2c2cd into crawl:master Aug 14, 2017
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