[#5394] Fix Concentration Check Logic to Address Multiple Issues #5424

Division4Studios · 2025-03-27T03:40:44Z

[#5394] Fix Concentration Check Logic to Address Multiple Issues

Updated Description

Problem

The concentration check logic in _onUpdate has undergone several iterations to address different issues, but the current implementation still has edge cases that need to be resolved. Specifically:

Original Issue: The condition (curr.value < curr.effectiveMax) was added in a previous PR (Concentration check triggered when reverting hp.tempmax and the actor's hp.value is higher than hp.max #3955) to prevent concentration checks from triggering when temporary HP (tempHP) was reduced but no "real" HP damage occurred. However, this condition caused concentration checks to fail in valid scenarios where temporary HP was reduced, as effectiveMax does not account for temporary HP.
Manual HP Adjustments: Moving the concentration check logic entirely to applyDamage was considered but raised concerns about manual HP adjustments (e.g., fall damage) not triggering concentration checks.
Edge Cases: The logic needed to balance handling temporary HP reductions, manual HP adjustments, and ensuring concentration checks align with the rules.

Solution

To address these issues, the concentration check logic has been updated as follows:

Restored Logic to _onUpdate:
- The concentration check logic remains in _onUpdate to ensure that all HP changes, including manual adjustments, are evaluated for concentration checks.
- This approach aligns with the original design intent and ensures consistency.
Removed Faulty Condition:
- The condition (curr.value < curr.effectiveMax) was removed because it fails to account for temporary HP and prevents valid concentration checks when temporary HP is reduced.
Simplified and Improved Logic:
- Concentration checks are now triggered based on whether the total HP change (changes.total) is negative, ensuring that any reduction in HP (including temporary HP) is evaluated.
- The condition (changes.temp < 0 || curr.value < curr.effectiveMax) was retained to ensure that temporary HP reductions are handled correctly while still respecting the original intent of PR Concentration check triggered when reverting hp.tempmax and the actor's hp.value is higher than hp.max #3955.
Tested for Edge Cases:
- The updated logic has been tested to ensure it works correctly for scenarios involving temporary HP reductions, manual HP adjustments, and other edge cases.

Final Implementation

Here is the updated concentration check logic:

// Determine if concentration check should be made
const isConcentrationEnabled = !game.settings.get("dnd5e", "disableConcentration");
const isUserInitiated = userId === game.userId;
const isDamageTaken = changes.total < 0;
const isConcentrationCheckAllowed = options.dnd5e?.concentrationCheck !== false;
const isHpReduced = changes.temp < 0 || curr.value < curr.effectiveMax;

if (isConcentrationEnabled && isUserInitiated && isDamageTaken && isConcentrationCheckAllowed && isHpReduced) {
  // Make the check using the total as input for the concentration DC.
  this.challengeConcentration({ dc: this.getConcentrationDC(-changes.total) });
}

Why This Approach?

Consistency:
- Keeping the logic in _onUpdate ensures that all HP changes, including manual adjustments, are evaluated for concentration checks.
- This aligns with the original design intent and avoids breaking existing functionality.
Handling Temporary HP:
- Removing (curr.value < curr.effectiveMax) resolves the issue where temporary HP reductions were not triggering concentration checks.
- The condition (changes.temp < 0 || curr.value < curr.effectiveMax) ensures that temporary HP reductions are handled correctly.
Flexibility:
- This approach balances the need to handle temporary HP reductions, manual HP adjustments, and other edge cases without introducing unnecessary complexity.
Community Feedback:
- The updated logic incorporates feedback from previous discussions and PRs, ensuring that it addresses the concerns raised by contributors.

Impact

Concentration checks now trigger correctly for all valid scenarios, including temporary HP reductions and manual HP adjustments.
The logic is simpler, more maintainable, and aligns with the official rules for concentration checks.
Edge cases from previous implementations are resolved, ensuring consistent behavior.

Let me know if you need further refinements!

Swaps the three-dots button with the delete button when holding shift rather than adding a new button to prevent layout shifts. Also adds the fixed width style to the delete button. Closes foundryvtt#5290

* Update system.json for 4.4.0 release * Implement UI for pending livestock trades. * Address PR feedback - Remove redundant max check. - Fix CSS rule. - Add faded-ui to bastion turn button in v13. - Fix deprecation warning. - Fix double-preparation in v12. --------- Co-authored-by: Jeff Hitchcock <mail@command-center.org>

Removes all of the unused item templates that were only used by the old item sheet plus several partials that are no longer used. Also moves `StartingEquipmentConfig` into a `config` folder to match the folder structure for actors and removed the two stub classes for `EnchantmentConfig` & `SummoningConfig`.

Fixes `EffectsElement`, `InventoryElement`, and `ItemListControlsElement` to work in old and new applications. Closes foundryvtt#5348

The `actorValues` method will now use the actor's source data rather than derived data when building its results. This means the trait config apps will not display derived values and that `TraitAdvancement` will no longer respect AE-assigned proficiencies when leveling. Closes foundryvtt#5334

Closes foundryvtt#5320

Don't display average damage in enrichers if the damage formula and the average damage are the same, preventing it from displaying `1 (1) piercing` when the extended enricher is used. Closes foundryvtt#5318

Positive numeric terms were adding an operator term while negative were not. This changes it so all numeric terms get an operator. Closes foundryvtt#5321

Adjusts the colors the embedded stat blocks for dark mode. Closes foundryvtt#5033

Currently "special" table of contents that append to an existing chapter won't appear if `showPages` is `false` for that chapter. This change moves some of the page display logic into the app and allows for displaying those special pages while hiding all other pages in 10000 the chapter.

Removes a lot of code that was deprecated and targeted for removal in `4.4` or earlier. Moves the removal target for most of the roll and activity changes to `4.5` at the easlier to avoid removing these shims to early given the quick release of `4.3`.

Since the vehicle sheet still uses many of the activity shims and probably won't be replaced until `5.0`, this bumps the removal version for those methods up.

Fixes some deprecation warnings caused by vehicle sheets accessing pre-activity values. Also fixes a bug where vehicles without any movement speeds would show `undefined ft` as their movement.

…led cantrips Switches to using `getDamageConfig` rather than using the raw damage parts when generating the damage labels. This allows the de-duplication in handling for `@mod` and other bonuses. This also means the scaled cantrip damage is displayed, rather than just showing the base damage. Closes foundryvtt#5275 Closes foundryvtt#5319

…styles Convert `AdvancementMigrationDialog` to use `Dialog5e` and the V2 styles.

Adjusts the system's art mapping to use core's system rather than the system's. Add handling to the system to ensure credit is added to actor descriptions when using the core's system. Deprecates the `ModuleArt` system while ensure it continues to work in tandem with core's system until the deprecation period ends. The removal version is set to `6.0` to give premium modules plenty of time to move over. Closes foundryvtt#5332

…eDialog` Deprecates the V1 `AdvancementSelection` dialog in favor of using `PseudoDocument#createDialog` same as activities. To fully support using advancements with this method a few changes were made: - Added support for proper coloring of SVG icons - Added optional `hint` that is added as tooltip - Added ability to disable options without removing them - Split some logic out from the `createDialog` method into `_createDialogTypes` and `_createDialogData` methods so each type can customize the dialog behavior

Redesigns the old polymorph dialog into a new `TransformDialog` that uses `ApplicationV2` and V2 styles. This new dialog displays name and tokens for the actor being transformed and the source actor in the header. On the left it has a list of presets defined in config. When a preset it selected it adjusts the checkboxes, allowing for further customization on top of the preset. The specific settings are now split into groups to make their functionality clearer. If a specific checkbox prevents another option from being used, then that other option will be locked. The data structure holding the settings has been modified to move away from the design of one giant object. Now settings are split into sets that match the grouping in the transform dialog. The setting has been changed to be a `DataModel` rather than a basic object. The old setting data has been discarded rather than migrated because it is easily replicated by players. The `transformInto` method still works off the old settings data structure, but a future PR will work on migrating that to the new setup. The configuration data for transformation settings and presets has been completely reworked to support more configuration data for each option and to make the presets match the new data structure for the settings. Closes foundryvtt#3877

…ddTemp` Adds a new formula field to define how much temp HP the transformed actor gets. The roll data in this field provides both the target actor's roll data as well as the source actor. This allows the polymorph preset formula to be `@source.attributes.hp.max`. This PR also adds information on subclasses to the roll data, using a `Proxy` setup that allows for determining whether a specific subclass exists in roll data (so `@subclasses.moon` will resolve to `1` if the player has the Moon Druid subclass, or `0` if they do not). This allows setting the 2024 wild shape temp HP formula to be `max(@classes.druid.levels, @subclasses.moon.levels * 3)`. Closes foundryvtt#5259

Adds a new activity type that allows transforming one or more actors into a another actor. The activity has two configuration tabs similar to summons, one for setting up profiles that control what actors can be selected to transform into, and a second that allows selecting a transformation preset and customizing it if desired. Currently only compendium browser-based actor selection is supported, but more modes can be added in the future such as direct linkage by UUID. When the transform activity is used the usage dialog will appear if more than one valid profile is present. After the usage is complete the compendium browser will open allowing for the player to select which creature should be used as the transformation source. Both the profile and source actor UUID are saved in the chat message. The chat message then has a "Transform" button that will transform selected tokens using the source actor and settings. Closes foundryvtt#4059

… in V13

…lable

…t#5333) * Porting inline style blocks used in content modules Adds v2/inline-blocks for compartmentalization and easier reference by users. * Does not accommodate dark mode yet Moves .notable to this file from v2/typography * Converts pixel padding/margins to `em` for better fit in item sheets and chat cards * Changes p:last-of-type selector removing margins to > :first/last-child for top and bottom margin * Changes notable headers to roboto-slab small-caps to emulate print style and visually differentiate * Header font-family converted to variable for easier restyling Renames `--dnd5e-block-*` to `--dnd5e-*` in an effort to shorten variable name length * Moving/updating quest, advice, narrative to inline-blocks Combined common background and border color variables for these blocks * notable modifies these vars in lieu of individual vars Applied hN styling of notables to these blocks Narrative changes: * converted corner dot size to em units to help visually isolate this block at high font sizes * Simplified creation of corner dots Quest/Advice changes: * gold icon border now also applied to non-round class img's Additionally, updated caption-top embed class to work with all document embeds using figcaption * Adding common position/size/layout inline blocks Positioning: left, right, clear-both, center * float left/right, clearing float, and centering via auto margin * float blocks' max-width limited to 50% container width (--dnd5e-block-float-max) Sizing: one, two, three, four * static width values as multipliers of --dnd5e-block-width-base (100px) * 100px, 200px, 300px, 400px (respectively) by default Layout: space-between/around, wrap, top * Flex row display helpers * default center alignment * top sets `align-items: start` * Adds darkmode support to inline callout blocks The notable border decorations do not appear to be affected by the grayscale filter applied in darkmode. I left this rule in as a comment with TODO to revisit (likely via clip path polygons). * Disambiguating postion/size/layout inline classes * left: float-left * right: float-right * clear-both: float-clear * center: margin-center * one, two, three, four: size-one, size-two, etc * Added local width override (helpful for MM usage) under each variant as `--local-width` * space-[around|between], wrap: flex-[around|between|wrap] * top: align-start * Renaming inline-blocks to blocks * Re-added dark mode notable decor grayscale filter. * Code style pass --------- Co-authored-by: Kim Mantas <kim.mantas@gmail.com>

Needed to Move Concentration Check from Update Actor Call to apply damage call. Concentration should happen on damage not adjusting health as loosing a temp hp buff should not proc concentration check.

Added Inline Comments

thatlonelybugbear · 2025-03-27T04:35:46Z

Why would there be a need to disallow concentration challenges when the HP are manually updated?

If there is a need to alter the actor's HP manually and not trigger concentration check, you can just ignore the chat message.

I think there are more instances that this is needed than not, a quick example, altering HP due to fall damage.

krbz999 · 2025-03-27T09:44:03Z

The current implementation was intentional.

Moving it out would also prevent players from triggering concentration.

Division4Studios · 2025-03-27T12:10:32Z

@thatlonelybugbear

That is actually a very valid point. I updated it to this implementation because found it annoying during testing to see the messages pop up when I was just resetting temp hp back to 0 over and over during testing. But you raise a good point. Convenience/Utility over Correctness.

My first implementation was actually fixing it simply to also register when lowering temp/normal hp. But then found the concentration popup annoying. However thats just anecdotal and from my experience doing some repetitive testing.

I appreciate the feedback from each of you. As well as the quick response.

@thatlonelybugbear and @krbz999

I will update the pull request and the description. I will move the implementation back to on update with an adjustment to allow temp HP adjustments to trigger concentration checks as well.

Updated Based on Conversation foundryvtt#5424

Division4Studios · 2025-03-27T12:30:59Z

Description, Title, PullRequest All Updated

thatlonelybugbear · 2025-03-27T12:57:59Z

I need to add that the curr.value < curr.effectiveMax check had been added so that it fixed another issue.

See #3955 and PR
Which had been closed via commit adding the < check.

I think that it needs maybe some check for

if (!game.settings.get("dnd5e", "disableConcentration") 
    && (userId === game.userId) 
    && (changes.total < 0) 
    && (options.dnd5e?.concentrationCheck !== false)
    && (changes.temp < 0 || curr.value < curr.effectiveMax)) {
  this.challengeConcentration({ dc: this.getConcentrationDC(-changes.total) });
}

edit: seems to work fine from some quick tests

### **Title**: Refactor Concentration Check Logic to Address Multiple Issues --- ### **Updated Description** #### **Problem** The concentration check logic in `_onUpdate` has undergone several iterations to address different issues, but the current implementation still has edge cases that need to be resolved. Specifically: 1. **Original Issue**: The condition `(curr.value < curr.effectiveMax)` was added in a previous PR (foundryvtt#3955) to prevent concentration checks from triggering when temporary HP (`tempHP`) was reduced but no "real" HP damage occurred. However, this condition caused concentration checks to fail in valid scenarios where temporary HP was reduced, as `effectiveMax` does not account for temporary HP. 2. **Manual HP Adjustments**: Moving the concentration check logic entirely to `applyDamage` was considered but raised concerns about manual HP adjustments (e.g., fall damage) not triggering concentration checks. 3. **Edge Cases**: The logic needed to balance handling temporary HP reductions, manual HP adjustments, and ensuring concentration checks align with the rules. #### **Solution** To address these issues, the concentration check logic has been updated as follows: 1. **Restored Logic to `_onUpdate`**: - The concentration check logic remains in `_onUpdate` to ensure that all HP changes, including manual adjustments, are evaluated for concentration checks. - This approach aligns with the original design intent and ensures consistency. 2. **Removed Faulty Condition**: - The condition `(curr.value < curr.effectiveMax)` was removed because it fails to account for temporary HP and prevents valid concentration checks when temporary HP is reduced. 3. **Simplified and Improved Logic**: - Concentration checks are now triggered based on whether the total HP change (`changes.total`) is negative, ensuring that any reduction in HP (including temporary HP) is evaluated. - The condition `(changes.temp < 0 || curr.value < curr.effectiveMax)` was retained to ensure that temporary HP reductions are handled correctly while still respecting the original intent of PR foundryvtt#3955. 4. **Tested for Edge Cases**: - The updated logic has been tested to ensure it works correctly for scenarios involving temporary HP reductions, manual HP adjustments, and other edge cases. #### **Final Implementation** Here is the updated concentration check logic: ```javascript // Determine if concentration check should be made const isConcentrationEnabled = !game.settings.get("dnd5e", "disableConcentration"); const isUserInitiated = userId === game.userId; const isDamageTaken = changes.total < 0; const isConcentrationCheckAllowed = options.dnd5e?.concentrationCheck !== false; const isHpReduced = changes.temp < 0 || curr.value < curr.effectiveMax; if (isConcentrationEnabled && isUserInitiated && isDamageTaken && isConcentrationCheckAllowed && isHpReduced) { // Make the check using the total as input for the concentration DC. this.challengeConcentration({ dc: this.getConcentrationDC(-changes.total) }); } ``` #### **Why This Approach?** 1. **Consistency**: - Keeping the logic in `_onUpdate` ensures that all HP changes, including manual adjustments, are evaluated for concentration checks. - This aligns with the original design intent and avoids breaking existing functionality. 2. **Handling Temporary HP**: - Removing `(curr.value < curr.effectiveMax)` resolves the issue where temporary HP reductions were not triggering concentration checks. - The condition `(changes.temp < 0 || curr.value < curr.effectiveMax)` ensures that temporary HP reductions are handled correctly. 3. **Flexibility**: - This approach balances the need to handle temporary HP reductions, manual HP adjustments, and other edge cases without introducing unnecessary complexity. 4. **Community Feedback**: - The updated logic incorporates feedback from previous discussions and PRs, ensuring that it addresses the concerns raised by contributors. #### **Impact** - Concentration checks now trigger correctly for all valid scenarios, including temporary HP reductions and manual HP adjustments. - The logic is simpler, more maintainable, and aligns with the official rules for concentration checks. - Edge cases from previous implementations are resolved, ensuring consistent behavior. --- Let me know if you need further refinements!

Division4Studios · 2025-03-28T00:53:45Z

@thatlonelybugbear That worked perfectly as far as my testing. Cleaned up the implementation for easier to understand variables so if future work needs to be done its abit easier to understand. Updated the PR and Description.

Division4Studios · 2025-03-28T00:56:15Z

Also thank you @thatlonelybugbear and @krbz999 for being patient with me on my first open source PR here. Hopefully i'll become more familiar with the overall project and history and be able to be a regular contributor.

module/documents/actor/actor.mjs

Co-authored-by: Zhell <50169243+krbz999@users.noreply.github.com>

arbron and others added 30 commits March 26, 2025 18:41

Update system.json for 4.4.0 release

63ce13e

Improve documentation for item types

fcef31a

[foundryvtt#5290] Swap chat control with trash button on holding shift

709ce0d

Swaps the three-dots button with the delete button when holding shift rather than adding a new button to prevent layout shifts. Also adds the fixed width style to the delete button. Closes foundryvtt#5290

[foundryvtt#5348] Fix custom elements in ApplicationV2

c9f52d1

Fixes `EffectsElement`, `InventoryElement`, and `ItemListControlsElement` to work in old and new applications. Closes foundryvtt#5348

[foundryvtt#5320] Add blank option for NPC spellcasting

40c3119

Closes foundryvtt#5320

[foundryvtt#5318] Don't show average damage if damage is flat

49f02a1

Don't display average damage in enrichers if the damage formula and the average damage are the same, preventing it from displaying `1 (1) piercing` when the extended enricher is used. Closes foundryvtt#5318

[foundryvtt#5321] Fix inconsistent spacing in simplified roll formulas

535703d

Positive numeric terms were adding an operator term while negative were not. This changes it so all numeric terms get an operator. Closes foundryvtt#5321

Fix error showing chat messages without delete buttons

5717f71

[foundryvtt#5033] Add dark mode styling for embedded stat blocks

7f551cf

Adjusts the colors the embedded stat blocks for dark mode. Closes foundryvtt#5033

Style pass

843dea6

Convert source source files from JSON to YAML

b72d2d2

Push activity & rolling deprecations to 5.0

57eaa46

Since the vehicle sheet still uses many of the activity shims and probably won't be replaced until `5.0`, this bumps the removal version for those methods up.

Fix activity deprecation warnings on vehicle sheet

c919c6f

Fixes some deprecation warnings caused by vehicle sheets accessing pre-activity values. Also fixes a bug where vehicles without any movement speeds would show `undefined ft` as their movement.

[foundryvtt#5319] Add deprecation warning to prepareDamageParts

58c4d59

[foundryvtt#4708] Redesign AdvancementMigrationDialog for V2 app & …

9d8005a

…styles Convert `AdvancementMigrationDialog` to use `Dialog5e` and the V2 styles.

[foundryvtt#4708] Add padding to hint, fix double drop on Firefox

cd3fe9d

Update system.json for 4.4.0 release

15ab9c5

[foundryvtt#5332] Fix reference to old suppressArt property

254d4e5

[foundryvtt#4708] Fix JSDoc issue

0be1070

Remove logging statement

9f00dac

[foundryvtt#3877] Fix title, uncheck disabled fields

42d7183

arbron and others added 8 commits March 26, 2025 18:42

[foundryvtt#5259] Standardize transform actor naming, remove Proxy

ca89bfc

[foundryvtt#4059] Fix bug with customizing settings, fix localization…

7f8ff0d

… in V13

[foundryvtt#4059] Ensure dialog shows when multiple profiles are avai…

f1ff56c

…lable

Revert "[foundryvtt#5332] Migrate to core's CompendiumArt system"

df68bbe

Update actor.mjs

936bc6d

Needed to Move Concentration Check from Update Actor Call to apply damage call. Concentration should happen on damage not adjusting health as loosing a temp hp buff should not proc concentration check.

Division4Studios changed the title ~~Fix: Concentration Check Triggered Only on Damage #5394~~ [#5394] Fix: Concentration Check Triggered Only on Damage Mar 27, 2025

Division4Studios changed the title ~~[#5394] Fix: Concentration Check Triggered Only on Damage~~ [#5394] Fix: Concentration Check Is Not Prompted When Damage is Only Applied to Temp HP Mar 27, 2025

Update actor.mjs

cdfe697

Added Inline Comments

Update actor.mjs

fc8a711

Updated Based on Conversation foundryvtt#5424

Division4Studios changed the title ~~[#5394] Fix: Concentration Check Is Not Prompted When Damage is Only Applied to Temp HP~~ [#5394] Fix Concentration Check Logic for HP Updates in _onUpdate Mar 27, 2025

arbron assigned Division4Studios Mar 27, 2025

arbron added the bug Functionality which is not working as intended label Mar 27, 2025

Division4Studios changed the title ~~[#5394] Fix Concentration Check Logic for HP Updates in _onUpdate~~ [#5394] Fix Concentration Check Logic to Address Multiple Issues Mar 28, 2025

krbz999 reviewed Mar 28, 2025

View reviewed changes

module/documents/actor/actor.mjs Outdated Show resolved Hide resolved

Update module/documents/actor/actor.mjs

1d01b0a

Co-authored-by: Zhell <50169243+krbz999@users.noreply.github.com>

Fyorl force-pushed the 4.4.x branch 2 times, most recently from 7f3a393 to d6936bb Compare April 4, 2025 14:48

Fyorl force-pushed the 4.4.x branch from d6936bb to 6dab519 Compare April 16, 2025 18:41

arbron force-pushed the 4.4.x branch from 97e82e3 to cacaffc Compare April 17, 2025 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[#5394] Fix Concentration Check Logic to Address Multiple Issues #5424

[#5394] Fix Concentration Check Logic to Address Multiple Issues #5424

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[#5394] Fix Concentration Check Logic to Address Multiple Issues #5424

Are you sure you want to change the base?

[#5394] Fix Concentration Check Logic to Address Multiple Issues #5424

Uh oh!

Conversation

Uh oh!