Add support for spellcast command's -x flag for all spells #293
Conversation
| @@ -56,25 +56,35 @@ void MagicSightSpell::Launch() { | |||
| m_hasDuration = m_launchDuration >= 0; | |||
| m_duration = m_hasDuration ? m_launchDuration : 0; | |||
|
|
|||
| ARX_SOUND_PlaySFX(g_snd.SPELL_VISION_START, &m_caster_pos); | |||
| bool emitsSound = !(m_flags & SPELLCAST_FLAG_NOSOUND); | |||
There was a problem hiding this comment.
this bit flag magic could be moved to a macro or function instead. makes it easier to understand
bool emitsSound = GET_EMITS_SOUND(m_flags);
also removes code repetition
There was a problem hiding this comment.
I think a member function would look much cleaner than a macro. I lean towards keeping the flag checks as they are, they are fairly clear.
There was a problem hiding this comment.
there is a double negation in there (NO and !) so they are much harder to read than "getEmitsSound"
a macro would mean no extra call to a function for such a simple operation
There was a problem hiding this comment.
As far as I'm concerned, macros are just preprocessor crutches. I wouldn't worry about efficiency as the function would probably get inlined, and this is not a hot codepath anyway.
There was a problem hiding this comment.
I would also lean towards a method in the Spell class as opposed to adding a macro because macros are leaning the codebase back towards C and low level constructs while a method could be migrated more easily to a more modern language if needed.
|
|
||
| if(m_caster == EntityHandle_Player) { | ||
| player.m_improve = true; | ||
| m_snd_loop = ARX_SOUND_PlaySFX_loop(g_snd.SPELL_VISION_LOOP, &m_caster_pos, 1.f); | ||
| if(emitsSound) { | ||
| m_snd_loop = ARX_SOUND_PlaySFX_loop(g_snd.SPELL_VISION_LOOP, &m_caster_pos, 1.f); |
There was a problem hiding this comment.
could all or some of these ARX_SOUND calls get a conditional variant where you pass the bool in? yes of course its more efficient to put the if() around each call instead of doing a method call just to jump back immediately, but i dont think this would affect anything.
just easier than to have 500 if(emitsSound) clauses in each spell file.
alternatively, create a macro
#define conditional_call(condition, func) if(condition){func;}
conditional_call(emitsSound, ARX_SOUND_PlaySFX_loop(g_snd.SPELL_VISION_LOOP, &m_caster_pos, 1.f))
There was a problem hiding this comment.
If I understand correctly, you'd pass a bool to a function just to tell it if it should do anything or not? That doesn't sound good.
It looks like the flag is set at the start of a spell and not anywhere else, so why not check the bit value once and use a member variable for that? Maybe see if you can use Spell base class to simplify what you want to do.
There was a problem hiding this comment.
Alternatively, ARX_SOUND_PlaySFX_loop() could be wrapped in a member function, say playSoundLoop() that checks for the member variable away from the main code, if reducing clutter is the goal.
There was a problem hiding this comment.
that playSoundLoop would do exactly what i proposed first with a function. the conditional macro thing would not add any extra function calls but make it a oneliner that is much easier to read. the files are long enough as they are
There was a problem hiding this comment.
I can't imagine reading that code a year later without having to inspect that macro and spending time on it. This doesn't feel like self-documenting code.
| @@ -460,9 +460,7 @@ void SlowDownSpell::End() { | |||
| m_targets.clear(); | |||
| } | |||
|
|
|||
| void SlowDownSpell::Update() { | |||
|
|
|||
| } | |||
There was a problem hiding this comment.
There seems to be more going on than what the commit message says. Please split formatting changes into a separate commit.
There was a problem hiding this comment.
I have restored the whitespace here and a few other places as it's not necessary for this PR. I'll try to split my commits in the future according to what you wrote.
dscharrer
force-pushed
the
master
branch
5 times, most recently
from
391b7f7
to
3d8a7e6
Compare
Currently only 4 spells (Heal, DetectTrap, Armor, LowerArmor) check whether the
SPELLCAST_FLAG_NOSOUNDflag is set. This addition adds support for all the sounds of all the spells.