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 for Adreneline CBM/Injector, Mycus wrath #14733

Merged
merged 8 commits into from Jan 15, 2016

Conversation

Projects
None yet
4 participants
@Malkeus
Copy link
Contributor

commented Jan 6, 2016

This adds a new..reference function? to effect.cpp/h. I needed to retrieve int_dur_factor for use in creature.cpp.
I then used the new function to force set the intensity if int_dur_factor > 0.

This resolves #14719 for real this time. After this change, there is no longer a debug message after using adrenaline cbm, injector or mycus wrath. Testing included adrenaline cbm, adrenaline injector and mycus wrath with some meth and cocaine as a chaser.

The bug was minor, but very noticeable when you needed the adrenaline rush to gtfo in a hurry. After activating or reactivating adrenaline cbm, adrenaline injector or mycus wrath(via seeing a plant monster), the intensity would be set to 0 for a single turn, giving you the stat and speed loss of adrenaline comedown and mycus respite. The next turn it would be caught by a sanity check and trigger a debug message, then function normally.

Still unresolved to my satisfaction: why does reupping with the adrenaline injector give 30 turns of duration instead of the expected 12? The cbm gives 7 as expected...vexing.

I had fun with this, and learned something. I hope this is the correct fix this time around ;)

@Malkeus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2016

lol...ignore the not declared in this scope bit, that was a panic'd earlier commit that i reverted. the update creature.h was also a reverted change that carried over from my local branch and I couldn't figure out how to remove. If either one will cause a problem, let me know and I'll do a clean branch.

@@ -802,6 +801,7 @@ void Creature::add_effect( efftype_id eff_id, int dur, body_part bp,
if (e.get_max_duration() > 0 && e.get_duration() > e.get_max_duration()) {
e.set_duration(e.get_max_duration());
}

This comment has been minimized.

Copy link
@Malkeus

Malkeus Jan 6, 2016

Author Contributor

Why?

@Malkeus Malkeus changed the title [WIP] [CR] Add_Effect Fix for Adreneline CBM/Injector, Mycus wrath Add_Effect Fix for Adreneline CBM/Injector, Mycus wrath Jan 6, 2016

@@ -814,6 +814,11 @@ void Creature::add_effect( efftype_id eff_id, int dur, body_part bp,
e.mod_intensity(e.get_int_add_val());
}

// Force intensity if it is duration based
if (e.get_int_dur_factor() != 0) {

This comment has been minimized.

Copy link
@BevapDin

BevapDin Jan 6, 2016

Contributor

Style issue: no space in front of the (, but a space after it. And another space before the ). See doc/CODE_STYLE.txt

Applies to the other added code as well. (Function declaration and definition are fine as they are now.)

And I agree with questions. Why are you adding and removing empty lines seemingly at random?

@Malkeus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2016

I will fix the style issues later today. The lines seem to be holdovers from the fiasco of my first attempt, I couldn't figure out how to make th4em go away short of starting over completely. I blame my own stupidity compounded by smartgit. I'm honestly thrilled that the only issues are style ones :)

edit: meh, i'll fix them now.

@@ -225,6 +225,8 @@ class effect : public JsonSerializer, public JsonDeserializer
bool get_harmful_cough() const;
/** Returns the percentage value by further applications of existing effects' duration is multiplied by. */
int get_dur_add_perc() const;
/** Returns the duration interval at which the effects intensity is decremented */

This comment has been minimized.

Copy link
@Malkeus

Malkeus Jan 6, 2016

Author Contributor

I feel like this isn't very informative, any suggestions?

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jan 12, 2016

Contributor

"Returns the number of turns it takes for the intensity to fall by 1 or 0 if intensity isn't based on duration.

This comment has been minimized.

Copy link
@Malkeus

Malkeus Jan 12, 2016

Author Contributor

Thank you, that's much better.

@Malkeus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2016

Should I style the rest of this function similarly? I notice it doesn't follow the same style. Also I'm not clear on how
e.set_intensity( ( e.get_duration() / e.get_int_dur_factor() ) + 1 ); ---This seems more legible to me.
should look. Is this acceptable or was the original ok?

original:
e.set_intensity((e.get_duration() / e.get_int_dur_factor()) + 1);

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2016

The version e.set_intensity( ( e.get_duration() / e.get_int_dur_factor() ) + 1 ); is correct.

Should I style the rest of this function similarly?

If you change large parts of a function, you could apply astyle before changing it. But here you only add a few lines, so it's not necessarily.

@Malkeus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2016

Ok, thank you, I'll keep that in mind. This is ready to for merging then.
On Jan 6, 2016 4:29 PM, "BevapDin" notifications@github.com wrote:

The version e.set_intensity( ( e.get_duration() / e.get_int_dur_factor()
) + 1 ); is correct.

Should I style the rest of this function similarly?

If you change large parts of a function, you could apply astyle before
changing it. But here you only add a few lines, so it's not necessarily.


Reply to this email directly or view it on GitHub
#14733 (comment)
.

@Malkeus Malkeus changed the title Add_Effect Fix for Adreneline CBM/Injector, Mycus wrath Fix for Adreneline CBM/Injector, Mycus wrath [CR] Jan 8, 2016

if( e.get_int_dur_factor() != 0 ) {
// + 1 here so that the lowest is intensity 1, not 0
e.set_intensity( ( e.get_duration() / e.get_int_dur_factor() ) + 1 );
}

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jan 12, 2016

Contributor

Why add it twice?
Can't existing effects be assumed to have correct intensity?

This comment has been minimized.

Copy link
@Malkeus

Malkeus Jan 12, 2016

Author Contributor

I assumed no, because of the existing check for bad intensity. I'm compiling without this check to see what happens.

This comment has been minimized.

Copy link
@Malkeus

Malkeus Jan 12, 2016

Author Contributor

It all seems to work fine, I'll delete this from my commit.

Malkeus added some commits Jan 12, 2016

Incorporated Coolthulhu's version.
Much more informative.
Removed unnecessary check
Per Coolthulhu

@Malkeus Malkeus changed the title Fix for Adreneline CBM/Injector, Mycus wrath [CR] Fix for Adreneline CBM/Injector, Mycus wrath Jan 12, 2016

@Malkeus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2016

Sleek and smooth, like a swimmer zombies legs.

@Rivet-the-Zombie Rivet-the-Zombie self-assigned this Jan 15, 2016

Rivet-the-Zombie added a commit that referenced this pull request Jan 15, 2016

Merge pull request #14733 from Malkeus/Add_EffectFix
Fix for Adreneline CBM/Injector, Mycus wrath

@Rivet-the-Zombie Rivet-the-Zombie merged commit fd7f842 into CleverRaven:master Jan 15, 2016

1 check passed

default
Details

@Malkeus Malkeus deleted the Malkeus:Add_EffectFix branch Jan 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.