Refactor the damage and resource pipelines #189
Conversation
|
After this latest iteration I feel more confident on the architecture, so I believe it's worth taking a look and now and giving feedback in order to improve the design. |
module/pipelines/damage-pipeline.mjs
Outdated
| [FU.affValue.vulnerability]: (damage) => damage * 2, | ||
| [FU.affValue.none]: (damage) => damage, | ||
| [FU.affValue.resistance]: (damage, { shift }) => (shift ? damage : Math.floor(damage / 2)), | ||
| [FU.affValue.immunity]: (damage, { shift, ctrl }) => (shift && ctrl ? damage : 0), |
There was a problem hiding this comment.
I was hoping we'd address the key modifiers on the next refactor.
I don't feel like modifier keys have any business being in logic layers. It's a UI-related issue and should be handled by the event consumer/hook. If in the future we somehow have other means of expressing "ignore resistance" that comes from a keyboard command rather than shift+click, we'd have to add more handler code in this logic layer.
The UI should be handled and the intent of "ignore resistance" is what should be passed to a pipeline imho.
There was a problem hiding this comment.
I completely agree with you and would like to strip this out on the next pass. I think with a "traits" system we should be able to express intents in an uniform way that both UI and other procedures can use.
| * @param {?} message | ||
| * @param {jQuery} jQuery | ||
| */ | ||
| function onRenderChatMessage(message, jQuery) { |
There was a problem hiding this comment.
nit: Feels like it ought to be in some UI helper method instead of pipeline but I also don't have any concrete suggestions. Not a big deal since it's purely organizational
…s, introduce traits
module/hooks.mjs
Outdated
| * @description Modify the result after it's been calculated. | ||
| * @example callback(context) { context.result += 42; return true; } | ||
| */ | ||
| DAMAGE_PIPELINE_CALCULATE: 'projectfu.pipelines.damage.calculate', |
There was a problem hiding this comment.
nit: naming might imply the step is for calculating. either change to past tense (calculated) or both steps to pre_calculate and post_calculate
There was a problem hiding this comment.
Very well; I was on the fence too on which tense to use for these hooks. But we can go with past-tense convention for the system.
| }, | ||
| ).render(true); | ||
| } | ||
| // import { FUActor } from '../documents/actors/actor.mjs'; |
There was a problem hiding this comment.
Ack, yes. Thanks.
| } | ||
|
|
||
| // TODO: Remove once users have migrated from legacy hooks | ||
| const beforeApplyHookData = new BeforeApplyHookData(request); |
There was a problem hiding this comment.
We don't use the result of the modified damage? The point of the hook is to be able to make changes to the calculation
module/hooks.mjs
Outdated
| * @example callback(context) { context.modifiers.set("foobar", 42); return true; } | ||
| * @example callback(context) { context.modifiers.delete("affinity"); return true; } | ||
| */ | ||
| DAMAGE_PIPELINE_PRE_CALCULATE: 'projectfu.pipelines.damage.collect', |
There was a problem hiding this comment.
nit: Change the underlying value to more closely match the const name. same with post
Refactors the damage and resource pipelines to be standardized and to provide a sequence of steps
pipelinesmodule, which now has the damage and resource pipelinesZero ShieldBonusesDataModel,DamageBonusesDataModelmodels to support skills such asDefensive Mastery,Symbol of Weakness.