PhysiCell PR party // briefing

Builds on #397's reverse-index idea but moves attacked_by into Cell_State (verified: copy_data and divide never copy state, and transformation explicitly preserves it — so both of 397's inheritance/wipe bugs vanish structurally). Adds a spring-style API (add_attacker/remove_attacker under the same unnamed omp critical the spring code uses, remove_all_attackers, remove_self_from_attacked) plus free functions begin_attack/end_attack that keep pAttackTarget, attacked_by and the spring in lockstep; standard_cell_cell_interactions now uses them instead of inline pointer writes. Teardown is hooked at every serial site: ~Cell, divide, both delete_cell variants, and convert_to_cell_definition. Separately, ingest_cell/fuse_cell/lyse_cell stop tearing down other cells' spring/attachment lists inline from within the mechanics parallel-for (a pre-existing race the author reports crashing 3/6 seeds of stock interaction-sample at 8 threads) and instead defer to the serial cells_ready_to_die pass, which runs before the position update, so the within-step behavior is unchanged. The MultiCellDS resume path now rebuilds all three records via begin_attack instead of just the pointer. It also fixes mutual-attack spring sharing (two attacks share one deduped spring; it is now only dropped when neither direction needs it) and caches the pAttackTarget read once before attack_cell to avoid a test-then-reload NULL window.

• Issue #395 is only partially fixed: a dying attacker still holds its target on a spring for the whole death phase (standard_cell_cell_interactions returns early for dead cells and no start_death hook exists); the UAF is gone because teardown happens at deletion, but the modeling complaint in #395 — release at death onset — remains open.
• Bundles several behavior changes that need ratification, not just review: (a) division now ends attacks in BOTH directions (attacker stops on divide, and everyone attacking a divider stops — resolution 1 of #396); (b) resumed simulations now recreate attack springs that were previously lost on save/load; (c) ingest/fuse/lyse teardown moves from inline to the serial removal pass.
• remove_all_attackers unconditionally calls detach_cells_as_spring for each attacker; if an attacker also had an independent dynamic spring to this cell they share one deduped spring, so a legitimate non-attack spring could be dropped — same semantics as development's unconditional detach at attack end, but worth knowing.
• Residual races the author documents but does not fix: mutual-attack begin/end interleaving can still strand a spring-less attack; attack_cell still writes damage through the target with no guard; several .size() reads remain outside the lock.
• Crash statistics (3/6 seeds on pristine development, 0/6 with deferral) are claimed, not independently reproduced by anyone else yet; no regression test or changes.md entry is included.

• Structurally eliminates both UAF directions (target deleted AND attacker deleted) rather than patching one; I verified the key claims against the code (state not copied on divide, dead-cell early return, serial-only cross-cell pAttackTarget writes)
• Follows an existing, familiar pattern (the spring API) instead of inventing one; future call sites cannot desync the three records

• Ratify #396 resolution 1: attacks end on division, in both directions. Is 'everyone attacking a divider stops' the biology the team wants, or should attacks-on-target survive target division?
• Decide whether to finish #395 here or as a follow-up: add an end-attack hook at start_death so a dying attacker releases its target immediately, not at deletion.
• Assign an independent reviewer (vincent-noel is the natural choice) and ask someone other than the author to reproduce the interaction-sample crash-rate numbers on development vs this branch.
• Settle the merge order for the trio: 422 subsumes 341 and 397 and conflicts textually with both (same insertion points in convert_to_cell_definition and delete_cell) — pick one path, don't merge piecemeal.

Opened 2026-08-06, updated 2026-08-08 — fresh. Mergeable CLEAN. No human review yet beyond drbergman's own inline comments (empty summary bodies); Copilot's one substantive concern (cross-thread remove_self_from_attacked from ingest/fuse) applies to an earlier revision — the final diff removed those inline calls, which is exactly the deferral fix. Author is drbergman himself, so the meeting is the review.

Related: #397 supersedes · #341 supersedes · #428 complements · #427 same-subsystem · issue #396 fixes · issue #395 partially-addresses

Fixes a real crash: pAttackTarget is never scrubbed when the target cell is deleted, so the attacker's next attack_cell() call writes through freed memory. The rescoped PR (originally it also did O(n) scans for asymmetric neighbors and springs; the author cut that after #409/#410) adds std::vector<Cell*> attacked_by to Cell_Interactions (i.e. inside phenotype), pushes the attacker onto the target's list when an attack starts in standard_cell_cell_interactions, erases it when the attack ends, and adds Cell::remove_self_from_attackers() to delete_cell(int) so a deleted target NULLs each attacker's pAttackTarget in O(#attackers). The diagnosis and the reverse-index idea are right — PR 422 explicitly builds on them — but the implementation has several holes that 422's writeup enumerates and that I verified independently in the code.

• attacked_by lives in phenotype, so convert_to_cell_definition's 'phenotype = cd.phenotype' wipes the target's list while attackers keep live pAttackTargets — the original use-after-free reopens for any transformed target (verified: transformation overwrites the whole phenotype).
• Cell::divide copies phenotype to the daughter ('child->phenotype = phenotype'), so the daughter inherits the parent's attacked_by; remove_self_from_attackers clears unconditionally with no 'pAttackTarget == this' check, so deleting the daughter silently aborts every attack aimed at the parent.
• No inverse teardown: when the ATTACKER is deleted, the target's attacked_by keeps a freed pointer; the guaranteed trigger is issue #395 — a dead attacker never runs its end-of-attack code because standard_cell_cell_interactions returns early for dead cells (verified guard at top of function) — and the later remove_self_from_attackers then WRITES through the freed attacker pointer. The PR converts …
• Thread safety: push_back/erase on the target's attacked_by happen inside the parallel interactions loop with no critical; two attackers picking the same target concurrently is a data race on a shared vector (springs, by contrast, are edited under criticals).
• MultiCellDS resume restores pAttackTarget but not attacked_by, so the fix is inert after a restart.

• Correct diagnosis of a real, reproducible crash, and the right algorithmic idea (reverse index, O(#attackers) not O(n) scan)
• Small, readable diff; author responsively rescoped it after community feedback and #409/#410 landed as separate fixes

• Decide 397 vs 422 explicitly, with vincent-noel in the room: 422 is his idea carried further, and closing 397 should come with clear credit (422's body already does this).
• If the team wants the smaller patch instead of 422: move attacked_by to Cell_State, add attacker-side teardown, guard the parallel list edits, and hook divide/convert — at which point it has converged on 422 anyway.
• Confirm whether anyone has been running this branch in production models (vincent-noel built it to fix real crashes in his sims — has it held up?).

Open since Dec 2025, last updated Mar 2026. Mergeable CLEAN. drbergman initially opposed it (thought it was an every-step O(n) scan), then reversed: 'this patch should be prioritized for merging' — but that was before he wrote 422. Three Copilot review passes (up to 11 comments on the pre-rescope version). Author's own comment says #397+#409+#410 together were 'a good fix' for the original crashes.

Related: #422 superseded-by · #409 complements · #410 complements · #341 same-subsystem · issue #395 related

Adds 8 lines to Cell::convert_to_cell_definition in core/PhysiCell_cell.cpp: if the transforming cell has a non-NULL phenotype.cell_interactions.pAttackTarget, it calls detach_cells_as_spring(this, pAttackTarget) before the phenotype is overwritten by the new cell definition. The pAttackTarget pointer itself needs no explicit clearing because the subsequent 'phenotype = cd.phenotype' assignment replaces it with the definition's NULL pointer (verified: cd.phenotype default-constructs pAttackTarget to NULL). Without this, the attack correctly ends on transformation (since 1.14.0) but the physical spring silently persists, dragging the ex-attacker around — demonstrated in the author's attached video (grey attacker transforms to yellow 'exhausted' cell yet stays tethered to the chemotaxing red target). The fix is correct in isolation: detach_cells_as_spring uses the same omp criticals as every other spring edit, and convert_to_cell_definition runs in the parallel phenotype loop where those criticals already protect concurrent spring edits.

• None in the code itself; the fix is minimal and sound against current development.
• It only covers the attacker-transforms case: it does not fix #395 (attacker death) or #396 (attacker division), and does not address the use-after-free class at all.
• Textually conflicts with PR 422, which inserts its own teardown (remove_self_from_attacked) at the exact same spot in convert_to_cell_definition — whichever merges second must rebase.

• 20-line-of-context, zero-risk change with a reproducible demo video plus config/cells.csv to recreate it
• Author's follow-up comment gives a precise code-level root cause (pAttackTarget lives in phenotype, which transformation overwrites without cleanup)

• Decide merge order: land 341 first (and rebase 422's convert hunk on top), or fold it into 422 and close 341 — they insert at the same line and cannot both merge cleanly.
• If merged standalone, note the (desirable) behavior change in changes.md: attack springs no longer persist after attacker transformation.
• Copilot nitpick: NULL vs nullptr — trivially ignorable or fixed on merge.

Open since Dec 2024 (oldest of the four). Mergeable CLEAN. No human reviews; only a Copilot nitpick (NULL vs nullptr). Author drbergman added a demo video and a detailed root-cause comment. Effectively waiting on the team to pick between it and PR 422.

Related: #422 superseded-by · #397 same-subsystem · #292 same-subsystem · issue #395 related · issue #396 related

One hunk in dynamic_spring_attachments (core/PhysiCell_standard_models.cpp): the detachment loop currently walks pCell->state.spring_attachments unlocked while it runs inside the mechanics parallel-for, where another thread handling a neighbor can call attach_cells_as_spring and push_back into THIS cell's vector under the unnamed omp critical. A reallocating push_back leaves the unlocked reader indexing a freed buffer — a genuine use-after-free crash. The fix snapshots the vector under that same unnamed critical and walks the copy; the lock is deliberately not held across the loop body because detach_cells_as_spring takes the same non-reentrant critical (holding it would deadlock). Stale snapshot entries are harmless: detaching an already-detached pair is a no-op, and attachments made mid-loop are just deferred one step in an already-stochastic process. Author reports 10/60 crashes on baseline vs 0/60 with the fix on a user model at 4 threads, at +2.4% wall time; deliberately leaves the attachment half's unlocked .size() reads and the non-atomic pair operations alone.

• None found in the hunk itself — the snapshot-under-the-writers'-lock pattern is correct, the backward iteration comment is preserved, and I verified the caller context (its own parallel-for in update_all_cells where attach/detach from sibling iterations are the only concurrent writers).
• The unnamed critical is program-global, so the per-cell snapshot copy serializes against every other unnamed critical in PhysiCell — correct by construction (it must match the writers) but the reported +2.4% is paid by all users with automated springs enabled; a shared NAMED critical across attach/detach/this-read would be the cleaner long-term fix.
• Function remains TSan-dirty by design (unlocked .size() reads, one-way-spring interleavings) — fine for a crash fix, but the PR should spawn a tracking issue so 'crash-free but not race-free' doesn't become the permanent state.
• Evidence comes from a private user model; there is no in-repo reproducer or test.

• Minimal, surgical, honestly scoped — the body explicitly lists what it does NOT fix
• Quantified evidence: 16.7% -> 0% crash rate over 60 runs per arm, cost measured two independent ways

• Ask drbergman why it is still draft — is he waiting on more benchmark arms, or on the 422 decision? If the fix is final, promote it.
• Is +2.4% wall time acceptable as a default cost for all spring-using models? (Likely yes for a crash fix, but say so on record.)
• Coordinate with heberlr's PR 427: its opt-in counter-based-RNG mode runs this same loop under 'omp ordered', which independently prevents this crash but only when opted in; 428 fixes the default path. Both can land; agree that 428 goes first and 427 rebases its spring-phase wrapper over it.
• Consider replacing the unnamed critical with one named lock shared by attach_cell_as_spring / detach_cell_as_spring / this snapshot in a follow-up, to stop serializing against unrelated criticals.

DRAFT, opened today (2026-08-09) — hours before the meeting. Mergeable CLEAN. No comments or reviews yet. Author drbergman; head branch name suggests it was extracted from the same investigation that produced PR 422.

Related: #422 complements · #427 same-subsystem · #397 same-subsystem

In core/PhysiCell_rules.cpp, canonicalizes behavior and signal names via the dictionary (find_behavior_index/behavior_name, find_signal_index/signal_name) everywhere the classic rules engine keys maps or compares by raw string: Hypothesis_Ruleset::add_behavior/find_behavior and Hypothesis_Rule::add_signal/find_signal/set_half_max/set_hill_power/set_response. Previously, two rules targeting the same behavior via different synonyms (e.g. 'cycle entry' vs 'exit from cycle phase 0') created two separate Hypothesis_Rule objects and whichever applied last silently overwrote the other's contribution instead of merging into one multivariate Hill response. Also fixes the latent `signals_map[signal] = signals_map.size()` bug by using `signals.size()` — under -std=c++11 the old line has unspecified LHS/RHS evaluation order, and map-size-as-index genuinely collides once a signal is added twice with opposite responses. Finally removes a dead 'wha?' code block in add_rule that reassigned the canonical behavior name back to the raw synonym, which under this PR would have actively undone the canonicalization. I verified Hypothesis_Ruleset::operator[] routes through the now-canonicalizing find_behavior, so add_rule's subsequent raw-synonym lookups still resolve correctly.

• Behavior change for models that (unknowingly) relied on the overwrite bug: synonymous rules now merge into one multivariate Hill response, so results change — toward correctness, but silently for affected users.
• Adding the same signal with the same response via two different synonyms now exit(-1)s at load (previously silently accepted as two entries); fails loudly, but previously-running (buggy) models will now refuse to start.
• Rule display/export (English_display, exported CSV) now shows canonical names rather than the user's original synonym — cosmetic, but round-tripping a rules file will rewrite names.
• Minor redundancy: set_half_max/set_hill_power/set_response canonicalize and then call find_signal which canonicalizes again — harmless, could be simplified.
• The fix does NOT extend to #370's new extended-rules module: PR 370's BehaviorRuleset::find_behavior (core/PhysiCell_rules_extended.cpp, ~line 1079) compares raw strings with no canonicalization, so the same synonym bug exists there.

• Fixes a real, silent wrong-results bug with a clear mechanism, plus a genuine C++11 unspecified-evaluation-order landmine, plus dead-code removal — all coherent and well-explained in the PR body
• Author attached a complete reproducer model (template project, cell_rules.csv/cells.csv/settings.xml) demonstrating before/after

• Flag to vincent-noel/heberlr/drbergman: #370's PhysiCell_rules_extended.cpp has the identical raw-string find_behavior bug this PR fixes in the classic engine — decide whether #370 must adopt canonicalization before it merges, or as an immediate follow-up.
• Merge order vs #370: same file, hunks are adjacent (add_behavior region) but non-overlapping — likely auto-merges, but whichever lands second should re-verify; merging #423 first is safer since it is small.
• Decide whether results-changing behavior (synonymous rules now merging) warrants a loud changes.md entry and/or a startup notice.
• Consider pairing with #356 in the same release: #356 fixes a synonym registration in the dictionary, #423 makes synonyms actually resolve to one rule.

Fresh (opened 2026-08-06). MERGEABLE / CLEAN. No reviews yet. One author comment attaching a complete reproducer model (rules CSV + cells CSV + settings XML). PR body is unusually thorough and accurately describes the diff, including correctly noting that rule *inputs* were not subject to the overwrite bug, only the shared lookup.

Related: #356 complements · #370 should-merge-before · #379 same-subsystem

Reorders `Cell::advance_bundled_phenotype_functions` in core/PhysiCell_cell.cpp so `apply_ruleset()` runs BEFORE `standard_cell_transformations()` (previously transformations ran first), and adds an `apply_ruleset(this)` call at the end of `Cell::convert_to_cell_definition()` so a freshly transformed cell has rules applied to its new phenotype immediately rather than waiting a full phenotype step. Motivation (documented with a worked example in PR comments): a rule that inhibits transformation was being applied only after the first transformation check, so at sim start ~60% of cells could transform in the very first step despite the inhibiting rule. Only 8 lines changed; the logic is simple and sound. The author notes an analogous ordering issue for gradient vectors that this PR deliberately does not address.

• Side effect: `convert_to_cell_definition` is also called by `standard_agent_legend` (modules/PhysiCell_pathology.cpp:1839) on a throwaway stack `Cell C` used only to draw the SVG legend. With rules enabled, `apply_ruleset` now runs on that dummy cell (registered at the origin voxel), so legend colors can shift if the coloring function reads rule-modified phenotype. Harmless in practice but …
• Behavior change by design: every rules+transformations model will produce different trajectories after merge (that is the point, but it should be flagged in changes.md).
• Copilot nit: the moved 'New March 2022' comment is now misleading given the reordering.

• Well-motivated with a concrete reproducible example (images in PR comments) showing the wrong pre-merge behavior and the fixed post-merge behavior
• Tiny, focused diff; thread-safe (apply_ruleset only reads the shared ruleset map and writes per-cell state, fine inside the OpenMP phenotype loop)

• Decide merge order vs #370: #370 renames apply_ruleset -> apply_behavior_ruleset in the same lines of advance_bundled_phenotype_functions, so whichever merges second must rebase; the new apply_ruleset call inside convert_to_cell_definition would also need renaming under #370.
• Should apply_ruleset in convert_to_cell_definition be skipped for the dummy legend cell (pathology.cpp)? A bool parameter or a check that the cell is registered would avoid rules running on a fake cell.
• Confirm the team is comfortable that all existing rules+transformation models will give different results; add a changes.md note.
• The author flagged the analogous gradient-vector ordering issue as unaddressed — decide whether to file it as an issue so it isn't lost.

Oldest assigned PR (opened Sep 2024, ~23 months old). MERGEABLE / CLEAN. No human reviews; only a Copilot review with one suppressed nitpick about a stale comment. Author (drbergman) self-commented 'can wait until after v1.14.0' and later added a detailed motivating example with before/after screenshots.

Related: #370 conflicts-with · #341 same-subsystem

Extends setup_signal_behavior_dictionaries(), get_signals(), and get_single_signal() in core/PhysiCell_signal_behavior.cpp with three new signals: 'current cycle phase' (with synonyms 'cycle phase', 'cycle phase index', 'current cycle phase index') exposing phenotype.cycle.data.current_phase_index, plus 'number of attachments' (state.attached_cells.size()) and 'number of spring attachments' (state.spring_attachments.size()). Indices are inserted mid-list (cycle phase before pressure; attachment counts before damage), which is safe because all lookups are dictionary-driven at runtime; map_index bookkeeping is handled correctly with no off-by-one, and signal_scales auto-sizes from int_to_signal.

• 'cycle phase' exposes the phase INDEX within the current cycle model, not the PhysiCell phase code: value 1 means different biology in Ki67-basic vs flow-cytometry models, and dead cells report indices of their death model. The PR body itself flags the dead-cell ambiguity. A Hill response on a categorical index is also dubious modeling; users will likely want equality semantics the rules grammar …
• Inserting signals mid-list shifts the numeric indices of every later signal (pressure, volume, contacts, damage, ...). Nothing in-core persists indices, but any user code or external tooling that hardcoded signal indices will silently read the wrong signal after upgrade.
• Attachment counts are read during rules evaluation without synchronization; if custom/phenotype functions attach or detach cells in the same parallel loop, the size() read can race (same class of issue PR #428 is fixing for springs). Low likelihood, worth a mention.
• User Guide / signal documentation not updated in the PR.

• Follows the existing code patterns exactly (dictionary entry + get_signals + get_single_signal, scaling applied consistently in both paths)
• map_index arithmetic verified correct, including replacing the old commented-out 'int map_index = m' oddity

• Decide the semantics of 'cycle phase': raw model-relative index (as implemented) vs PhysiCell_constants phase code vs not exposing it at all; document whatever is chosen, especially for dead cells
• Decide whether rules should even support categorical signals, or whether this belongs with the rules-upgrade work in #370 (which rewrites the same file — merge order matters: 370 and 385 both touch PhysiCell_signal_behavior.cpp/h)
• Ask for a User Guide addition listing the three new signals and synonyms
• Note the 9 failing 'windows (...)' checks are the repo-wide Windows CI breakage, not this PR

Opened 2025-08-19 (~1 year old), MERGEABLE/UNSTABLE — the failing checks are the same 9 Windows build_binaries jobs failing on every open PR (repo-wide CI issue). No reviews at all, human or Copilot. Author is drbergman.

Related: #370 conflicts-with · #385 conflicts-with · #422 same-subsystem · #428 same-subsystem · #423 same-subsystem

Adds a family of fix_substrate_at_voxel(s)/unfix_substrate_at_voxel(s)/fix_substrates_at_voxel overloads (by substrate name or index, single voxel or batch, with/without new values) to Microenvironment, all of which consistently maintain the three Dirichlet bookkeeping structures (per-voxel-per-substrate activation, per-substrate global activation, voxel.is_Dirichlet) that the legacy add/update/remove API handled inconsistently (issue #214). Adds find_existing_density_index() which exits with a clear error on a bad substrate name, and a sync_substrate_dirichlet_activation() helper. Rewrites set_microenvironment_initial_condition() boundary-condition setup to use the new API, skipping non-activated substrates. The legacy API is left in place for backwards compatibility, and the header's commented-out cruft around the Dirichlet members is cleaned up.

• sync_substrate_dirichlet_activation() iterates 'for (auto voxel_activation_vector : dirichlet_activation_vectors)' by value, copying every voxel's vector<bool>; should be const auto&. unfix_substrates_at_voxel() calls it once per substrate, making that path O(substrates^2 x voxels) with copies.
• Error handling is inconsistent: bad substrate name exits(-1) via find_existing_density_index, but size-mismatch in the batch fix functions only prints to cerr and silently returns, leaving the model running with unapplied values.
• Subtle behavior change in set_microenvironment_initial_condition(): boundary voxels no longer get Dirichlet values stored for substrates whose boundary activation is false (old add_dirichlet_node wrote the full value vector). If user code later activates such a substrate expecting the boundary value to be pre-loaded, it will see the stale default instead. Also is_Dirichlet is no longer set on …
• No unit tests accompany the new API despite unit_tests/custom_DCs_2substrates existing as a natural home.

• Directly resolves the long-standing API confusion documented in issue #214 with a clean, consistent mental model (fix/unfix)
• Keeps full backwards compatibility with the add/update/remove API

• Team sign-off on the fix/unfix naming and on keeping (vs deprecating with warnings) the old add/update/remove API — issue #214 can be closed once docs are updated
• Fix 'for (auto v : dirichlet_activation_vectors)' to const auto& before merge
• Decide error policy: should size mismatches in the batch functions exit like find_existing_density_index does, or keep the silent cerr+return?
• Confirm the initial-condition change (values no longer written for non-activated boundary substrates) is acceptable

Opened 2025-04-01 (~16 months old), MERGEABLE/CLEAN. Only Copilot review (one low-confidence naming nit: voxel_ind vs voxel_index). Author (drbergman) commented that accepting it should trigger rework of #357. No human reviews yet.

Related: #357 should-merge-before · #401 conflicts-with · #386 same-subsystem · #417 same-subsystem · #337 same-subsystem · issue #214 fixes · issue #414 related

Wires up the `<dirichlet_nodes type="csv" enabled="true"><filename>` element that has shipped in template configs for years but was unimplemented ('not yet supported'). The CSV mirrors the substrate-IC format (header x,y,z,<substrate subset>; one row per voxel), with a twist: an empty cell means 'leave that voxel-substrate DC untouched', a number sets the DC value and activates it. File-based DCs are applied AFTER XML boundary DCs, so the CSV can override values but never disable a DC. To do this it refactors set_microenvironment_initial_condition into set_dirichlet_initial_condition -> set_dirichlet_boundaries_from_XML / set_dirichlet_boundaries_from_file (the XML boundary code is moved verbatim, which is why the diff looks scarier than it is — the author annotated this in 6 self-review comments), adds Microenvironment::set_only_substrate_dirichlet_activation (updates the substrate-level display flag without clobbering per-voxel activations), and a new parser dirichlet_csv_to_vector that tracks missing entries. Along the way it fixes two real pre-existing bugs in the no-header substrate-IC CSV path (a loop counter that never incremented because `continue` skipped `i++`, and a shadowed std::ifstream that left the outer file closed, so no-header IC CSVs always died with 'wrong number of voxels'). Adds a demo sample project sample_projects/dirichlet_from_file (interior-voxel DCs with both empty-value styles) plus Makefile targets. The matlab file type is stubbed to error out.

• Unfixed Copilot finding: dirichlet_csv_to_vector (BioFVM/BioFVM_vector.cpp) increments `ind` with no bounds check — a data row with MORE fields than the header/first row causes out-of-bounds writes to is_missing/data (UB, possible heap corruption) instead of a clean error; the too-few-fields case is checked but only after the damage.
• Unfixed Copilot finding: the column-count warning in get_row_from_dirichlet_condition_csv prints microenvironment.number_of_voxels() as 'Expected' when the check is against number_of_densities()+3 (bug copied from the substrate-IC version, which has the same wrong message).
• std::stod on any non-numeric field throws an uncaught exception -> terminate with no useful message. Windows CRLF files are a concrete trigger: a trailing empty field arrives as "\r", which is non-empty so stod throws. PR #417 (trim_cr) exists precisely for this and should be applied to the new parser.
• Duplicate-voxel detection is an O(n^2) linear scan of voxel_set per row — fine for small DC files, pathological if someone supplies a whole-mesh CSV; #386 already switched the IC loader to an O(1)-per-row voxel_is_set bitmask.
• Semantic edge case: update_dirichlet_node marks the voxel is_Dirichlet=true; since per-voxel activation vectors are seeded from the substrate-level activation at init, a substrate whose XML Dirichlet_boundary_condition was enabled will ALSO become clamped at every voxel the CSV newly marks — a surprising cross-substrate side effect that is neither prevented nor documented.

• Fills a genuine, years-old gap: the dirichlet_nodes XML element exists in every template config but did nothing; arbitrary interior-voxel DCs are a frequently requested capability.
• Sensible override semantics (CSV applied after XML; empty = no change; can override values but not silently disable) that are clearly stated in the PR body and exercised by the sample dcs.csv.

• Pick ONE empty-cell convention: #386 (same author) makes empty entries in substrate-IC CSVs mean 0.0, while this PR makes empty entries in DC CSVs mean 'skip'. Both formats are pitched as 'the same structure' — decide whether to unify or explicitly document the divergence, and whether #357 and #386 should be combined into one CSV-handling PR.
• Decide merge order for the DC/CSV cluster: #369 ('simplified dc functions', same-day companion addressing issue #214) merges cleanly with #357 textually, but its new fix_substrate_at_voxel API is arguably what this loader should call — landing #369 first and porting #357 onto it avoids adding set_only_substrate_dirichlet_activation to an already-inconsistent DC API family.
• Sequence against #401 (BioFVM Interface): #401 restructures BioFVM and conflicts with this PR on BioFVM_microenvironment.h and PhysiCell_settings.cpp; if #401 is going in, small BioFVM PRs like this should land first or be rewritten on top.
• Should the CSV be able to turn a DC OFF (e.g. a sentinel like 'off' or NaN)? Current design can only set/override, which the PR body frames as intentional — confirm that is the desired contract.

CONFLICTING/DIRTY on GitHub, but merge-tree shows the only textual conflict with development is the trivial .gitignore hunk — everything else auto-merges. Created 2025-02-02, last activity 2025-05-22 (~15 months dormant); based on the 1.14.2 tag point while development has advanced ~71 commits (incl. CMake CI #392, resume-sim #400, Windows fixes). No human maintainer review yet: the 8 review comments are 6 …

Related: #386 conflicts-with · #369 should-merge-before/after · #401 conflicts-with · #417 complements · #421 complements · #372 same-subsystem · #371 same-subsystem · issue #214 related · issue #414 related

Relaxes load_initial_conditions_from_csv (BioFVM/BioFVM_microenvironment.cpp) so a substrate IC csv no longer has to specify every voxel and every substrate: omitted voxels and empty fields (',,') default to 0 (densities start zeroed, and when a CSV is enabled the XML initial_condition values were already ignored). Removes the hard error requiring exactly #voxels rows; replaces the O(n^2) duplicate-voxel check (linear scan of a growing vector) with an O(1) lookup in a vector<bool> sized to the mesh. Adds substrate_csv_to_vector in BioFVM_vector.cpp, a comma-splitting parser that maps empty fields to 0.0, replacing csv_to_vector for this path. Changes the signature of get_row_from_substrate_initial_condition_csv in the public BioFVM header.

• Blank lines: substrate_csv_to_vector('') returns a 1-element vector {0.0}, then position = {data[0], data[1], data[2]} reads data[1]/data[2] out of bounds — UB/garbage voxel index. A file ending in an extra newline+blank line hits this. Needs the same blank-line skip as #421 (pre-existing flavor of this bug existed, but the PR touches exactly this code)
• Short rows: a row with fewer physical fields than the header (e.g. '20,0,0,1' under x,y,z,sub1,sub2 — no trailing comma) makes data[ci+3] read out of bounds in the assignment loop; only trailing-comma-style missing values are safe. No per-row column-count validation when a header is provided (the one-time warning fires only in the headerless case)
• Silent semantics change: files that previously hard-errored for incomplete voxel coverage now load silently with zeros — a typo'd/truncated file is no longer caught. At minimum a summary line ('set N of M voxels; rest defaulted to 0') would help
• Pre-existing but untouched: the headerless branch is completely broken on development (shadowed ifstream reads a closed stream so no rows load; the i++ after continue leaves substrate_indices empty). This PR edits the surrounding function without fixing it — Copilot flagged the same on #417
• Based on Jan 2025 dev (71 commits behind); GitHub still reports MERGEABLE, but it textually conflicts with #417 in the read loop (verified with git merge-tree) and heavily conflicts with #357 (4 files)

• Genuinely useful UX: sparse CSVs (a hotspot of one substrate) no longer require enumerating every voxel and every substrate column
• vector<bool> duplicate detection is a real performance fix — the old linear scan was O(#voxels^2) and painful on large meshes

• Decide the default for omitted voxels: 0 (this PR) or the XML <initial_condition> value (arguably more intuitive — XML sets the background, CSV sets hotspots; currently enabling a CSV silently discards the XML values entirely, which surprises users)
• Decide whether losing the 'wrong number of voxels' hard error is acceptable, or whether a post-load summary/warning (N of M voxels set) should be added
• Author (drbergman) should add guards: skip blank lines (mirror #421) and validate per-row field count against the header before indexing data[ci+3]
• Unify substrate_csv_to_vector (this PR) with dirichlet_csv_to_vector (#357) — near-duplicate 'empty field = missing' parsers inserted at the same spot in BioFVM_vector.cpp; verified 4-file merge conflict between the two PRs. Decide which lands first and make the other reuse its parser

Opened Sept 2025 (11 months old), no reviews at all — not even Copilot. mergeStateStatus UNSTABLE (CI failing/pending), mergeable per GitHub but based on Jan 2025 code, 71 commits behind dev. Same author (drbergman) as #417/#421/#357, so coordination is easy — this is really a four-PR CSV-robustness cluster that should be sequenced at the meeting.

Related: #357 conflicts-with · #417 should-merge-after · #369 same-subsystem · #401 same-subsystem

Hardens the cell initial-condition CSV loaders in modules/PhysiCell_geometry.cpp. In load_cells_csv_v1, rows parsing to 0 values (blank lines, including trailing blank lines) are skipped instead of falling into the != 4 error. In process_csv_v2_line (v2 loader), 0-token lines return NULL (skipped) and lines with 1-3 tokens now exit with a clear 'expects at least 4 columns' error instead of the previous cryptic segfault (tokens[3] out-of-bounds when a row like '0,1,default' has only 3 fields). 12 added lines, 1 file.

• Without #417 merged first, a CRLF 'blank' line ('\r') is NOT caught: in v1, csv_to_vector's digit-scan loop walks past the null terminator on lines with no numeric characters (pre-existing buffer overrun, flagged by Copilot, acknowledged by the author with a pointer to #417); in v2 it becomes a 1-token line and hard-errors. So the blank-line fix is only complete when combined with #417
• Whitespace-only lines (' ') still produce 1 token in v2 and exit(-1) rather than being skipped — arguably acceptable (fail loud) but inconsistent with the blank-line skip
• Pre-existing and NOT fixed: v2 rows with MORE tokens than header labels index labels[k] out of bounds in the extra-data loop (Copilot flagged); a malformed wide row can still segfault
• Style nit: uses std::cerr where the rest of the file uses std::cout for errors

• Converts a genuinely cryptic segfault (misspecified row -> tokens[3] OOB) into an actionable error message
• Tolerates the very common trailing-blank-line and separator-blank-line cases in hand-edited CSVs

• Agree merge order: #417 first, then #421 (author already noted the dependency in his Copilot reply); they merge cleanly together
• Decide whether to also guard the pre-existing wide-row case in process_csv_v2_line (tokens.size() > labels.size() reads past labels) — 3 more lines while the file is open, or a follow-up
• Decide policy: should whitespace-only lines be skipped like blank ones, or is exit(-1) the desired fail-loud behavior?
• Longer-term: csv_to_vector in BioFVM_vector.cpp scans past the end of the buffer on lines with no numeric characters — pre-existing UB worth its own issue (author declared it out of scope here)

Opened July 2026 by drbergman, based on dev tip, CLEAN/MERGEABLE. Copilot review with one substantive inline comment (whitespace-only lines / csv_to_vector overrun); drbergman replied declaring it out of scope and pointing to #417 as the companion fix. No other human review.

Related: #417 should-merge-after · #372 same-subsystem · #401 same-subsystem · issue #272 partially-addresses

Replaces `Phenotype::intracellular` (a single `Intracellular*`) with `std::vector<Intracellular*> intracellulars`, moves the `pre_update_intracellular`/`post_update_intracellular` hooks off `Cell_Functions` onto each `Intracellular` object (with a new signature adding an `Intracellular*` argument), and changes `Intracellular::inherit(Cell*)` to `inherit(Intracellular*)`. Adds a new XML format `<intracellulars><intracellular type=...>...` (parsed after, and clearing, the old single-`<intracellular>` format, which is kept for back-compat) and updates `update_all_cells` to loop over each model per cell. Updates PhysiBoSS and libRoadrunner addons and two boolean sample projects to the new API, and ships a new `spheroid_tnf` sample that couples an SBML TNF-receptor ODE model (roadrunner) with a MaBoSS cell-fate Boolean model in the same cell. Importantly, it bundles wholesale copies of other open work: drbergman's librr rewrite (PR #362, commit 'daniel's librr') plus further changes (get/setFloatingSpeciesByIndex, per-model mapping lambdas), and rheiland's new libRoadrunner setup script (PR #406 — beta/setup_libroadrunner.py is byte-identical to #406). The author explicitly frames it as a proposal: '#362 and #406 should be focused on first' and dFBA integration awaits the BSC dFBA PR.

• New `<intracellulars>` parser (core/PhysiCell_cell.cpp) unconditionally clears pCD->phenotype.intracellulars, then the 'already copied from parent' branches index `pCD->phenotype.intracellulars[intracellular_count]` on that now-empty vector — out-of-bounds/segfault. Since pParent defaults to &cell_defaults for every non-default definition and the maboss/roadrunner guard is `pParent->...size() >= …
• In that same block, the roadrunner copy-branch passes `node` (the `<intracellulars>` wrapper) instead of `intracellular` to initialize_intracellular_from_pugixml; guards are inconsistent (`>=` for maboss/roadrunner, `>` for dfba); clear() drops parent clones without delete (leak).
• dFBA addon was NOT updated: addons/dFBA/src/dfba_intracellular.h still declares `inherit(PhysiCell::Cell*)` while the base pure virtual is now `inherit(Intracellular*)`, making dFBAIntracellular abstract — any ADDON_PHYSIDFBA build fails to compile (author acknowledges, waiting on BSC PR). The fba/cancer_metabolism sample also still uses `phenotype.intracellular`.
• sample_projects_intracellular/ode/ode_energy/custom_modules/custom.cpp line 154 still calls `pCell->phenotype.intracellular->set_parameter_value(...)` — the member no longer exists, so `make ode_energy` does not compile at the PR head.
• RoadRunnerIntracellular's copy ctor does not copy the new base-class pre/post_update_intracellular function pointers (MaBoSS's was fixed to do so), so user hooks set on a roadrunner cell definition are silently NULL in every cell created from it; spheroid_tnf doesn't notice only because its ODE hooks are empty.

• Solves a real, requested modeling need (hybrid Boolean + ODE + FBA per cell) with a mostly sensible design: per-model pre/post hooks and passing the specific Intracellular* into hooks/mappings is cleaner than the global per-cell hook
• Keeps old single-`<intracellular>` XML parsing for config back-compat

• Agree on merge order: land #362 (librr rewrite) and #406 (setup script) first — #415 literally contains both — and decide whether #425's rrHandle/result + unknown-species-fatal fixes get folded into #362 or #415; then vincent-noel rebases #415 down to a pure 'multiple intracellulars' PR.
• Decide the migration story for the API break: every PhysiBoSS/roadrunner user project uses `phenotype.intracellular` and `functions.pre/post_update_intracellular`. Options: keep a deprecated accessor (e.g. intracellulars[0] alias or get_intracellular(type)), or accept a hard break at a major version and document it in changes.md.
• The parent/child inheritance path of the new `<intracellulars>` parser must be redesigned (currently clears the inherited clones then indexes into the empty vector → segfault for any non-default cell definition; roadrunner branch also passes the wrong XML node). Decide: should child definitions match parent intracellulars by position, by type, or not inherit at all?
• Who updates the dFBA addon (inherit signature, phenotype.intracellulars) — fold into this PR or wait for the BSC dFBA PR from marcorusc/migp11 as vincent suggests? Until then ADDON_PHYSIDFBA builds fail.

MERGEABLE/CLEAN per metadata (no textual conflict with development yet, though it logically supersedes files touched by #362/#406/#425). No formal reviews; 3 comments, all positive-but-preliminary: drbergman ('really cool', wants a coordinated conversation on Intracellular updates, suggests a GitHub project), migp11 (BSC, enthusiastic, proposes Boolean+FBA hybrid use cases), author confirming the TNF model …

Related: #362 should-merge-before · #406 should-merge-before · #425 conflicts-with · #363 conflicts-with · #364 conflicts-with · #403 complements

Reworks the libRoadrunner ODE addon around a new RoadRunnerMapping class: each <map> element in the XML now declares type="input|output|io" and a physicell_name that can be any signal or behavior dictionary entry (or a legacy token like mms/sur_oxygen/ctr_0_1), and at first cell creation the addon builds a lambda per mapping that shuttles values between PhysiCell and the SBML model automatically before/after each intracellular step. The intracellular solver is now actually scheduled: update() integrates the real elapsed simulation time and advances next_librr_run by a per-cell-type intracellular_dt (defaulting to the previously-unused PhysiCell::intracellular_dt from <overall>), whereas the old code's need_update() was effectively always true and integrated a hard-coded 0.01 min. The ode_energy sample project is gutted of ~130 lines of manual glue (custom update_intracellular(), manual main.cpp scheduling) to demonstrate the new zero-custom-code workflow. It also fixes real bugs in the old token code: 'sur_*' always wrote uptake_rates[1] regardless of substrate, and death tokens hard-coded rates[0]/rates[1] instead of using find_death_model_index. Core changes are minimal: validation now also runs for inherited intracellular models in initialize_cell_definition_from_pugixml, plus debug-print removal (including the 'having trouble finding' message in find_signal_index).

• Hard breaking change beyond what the body claims: old-format <map PC_substrate=...> configs hit exit(-1) ('type must be io, input, or output') — old token NAMES survive but the old attribute format does not; every existing librr user's config breaks
• update_phenotype_parameters() is kept in the API but silently turned into a no-op (returns 0); existing user code that calls it (the old documented pattern) compiles fine and silently stops working
• RoadRunnerMapping objects are raw pointers shared across every cloned intracellular instance (copy ctor copies the vectors of pointers) with no ownership/destructor; re-running initialize_intracellular_from_pugixml leaks the previous mapping vectors — same leak-prone territory PR #425 is cleaning up
• parse_ctr_token: start index uses name.substr(4, pos-1) — wrong length, only works because atoi stops at '_'; the try/catch around atoi is dead code (atoi never throws), and malformed tokens like 'ctr_a_b' silently parse as {0,0} and pass validation, writing transition_rate(0,0)
• validate_SBML_species() still calls createRRInstance() per cell definition and overwrites rrHandle without freeing — now invoked for inherited definitions too, so slightly more leaked instances (overlaps/conflicts with #425)

• Genuinely removes the biggest pain of the ODE addon: no custom C++ glue needed to pass values between PhysiCell and SBML (sample custom.cpp -100 lines, main.cpp -27)
• Fixes real long-standing bugs: 'sur_*' token wrote uptake_rates[1] for every substrate; death tokens hard-coded rates[0]/[1]; need_update() previously never advanced so the solver ran every diffusion step with a hard-coded 0.01-min integration

• Roadmap decision first: this PR vs #415 (unified intracellular API, also rewrites librr_intracellular.*, MERGEABLE and newer) — if #415 wins, the mapping automation here should be re-ported on top of it rather than merged as-is
• Merge order: land the small recent librr fixes first (#425 leak/uninitialized handle, #406 setup+Makefile), then have drbergman rebase this onto development — it is CONFLICTING and based on the 1.14.2 tag
• Backward compatibility policy: old PC_* map attributes now exit(-1); decide between a compat shim with deprecation warning vs hard break with loud release notes, and confirm Studio sync (rheiland already flagged Studio changes needed at release)
• Resolve the open dt question from the PR thread: keep <dt_intracellular> in <overall> as a global default, or require per-cell-type <intracellular_dt> (author floated enforcing explicit per-type values)

Open since 2025-02-13 (18 months), mergeable=CONFLICTING / mergeStateStatus=DIRTY, branched off 1.14.2. Reviewer sentiment positive: rheiland (collaborator) 'really liking this so far', asked for Studio sync at release, an input-map demo, and questioned dt_intracellular in <overall>; author agreed and floated enforcing per-cell-type dt — thread went quiet after Feb 2025. Two Copilot review passes with only …

Related: #415 conflicts-with · #425 should-merge-before/after · #406 complements · #363 same-subsystem · #364 same-subsystem

Gives RoadRunnerIntracellular an owning lifecycle: rrHandle gets an in-class nullptr initializer, a destructor frees rrHandle and result, copy/assignment are deleted, and clone() now calls start() so every Phenotype assignment (divide, create_cell, and critically convert_to_cell_definition, which previously produced an indeterminate handle) yields a working RoadRunner instance. The two explicit start() calls in core/PhysiCell_cell.cpp are removed as redundant. Also hardens get/set_parameter_value to use map::find with a fatal error instead of operator[], which silently aliased unknown SBML species to column 0. Author's stress tests show crashes eliminated on type conversion and memory flat instead of growing ~5x under cell turnover.

• CI is red: PhysiBoSS Cell Lines and PhysiBoSS Tutorial validation steps fail on all 6 platforms (build and run succeed; outputs differ from golden references). Cause: removing start() from divide()/create_cell() is not behavior-neutral for MaBoSS. MaBoSSIntracellular::start() sets next_physiboss_run = max(start_time, current_time) and calls restart_node_values(); the copy ctor restarts node …
• RoadRunnerIntracellular::start() does not free an existing rrHandle before createRRInstance(), so any legacy user code still calling intracellular->start() manually after create_cell now leaks one instance per call
• Perf: Cell::Cell() assigns cell_defaults.phenotype, so when the default cell definition carries an SBML model every cell creation now does a full SBML load that is immediately discarded when create_cell assigns cd.phenotype - a second load. Correct but doubles an already expensive step
• Destructor calls rrc::freeRRInstance(rrHandle) on never-started instances (XML-parsed cell-definition objects) where rrHandle is nullptr - almost certainly safe in the rrc C API but worth one confirmation
• Verified true as claimed: Intracellular has a virtual destructor and Phenotype's operator=/destructor delete through the base pointer, so the new destructor genuinely runs on every death/conversion; the uninitialized-handle-on-convert_to_cell_definition bug is real at development head

• Fixes three genuine, verified bugs: indeterminate rrHandle after convert_to_cell_definition (crash), permanent leak of every RoadRunner instance on death/conversion, and silent column-0 aliasing for misspelled SBML species
• Deleted copy operations plus owning destructor is the right C++ shape; matches how MaBoSS/dFBA initialize in their own copy constructors

• Decide the fix strategy for the PhysiBoSS regression: (a) keep the core start() calls and make RoadRunner::start() free-then-create (idempotent), letting clone()+start coexist, or (b) drop core start() but make MaBoSS's copy ctor carry start_time/next_physiboss_run - option (b) still shifts RNG streams and needs golden-reference regeneration with vincent-noel's approval
• Reconcile with #362 (drbergman's own libRoadrunner API rewrite, 18 months old): #362 rewrites the same files and does NOT fix the leak or the uninitialized handle - decide whether #425 merges first as the bugfix and #362 rebases, or whether #425's destructor/clone-start gets folded into #362
• If start() stays removed from core, audit sample projects and docs for user code calling intracellular->start() manually (now a per-call leak given start() doesn't free)
• Note the doubled SBML load per cell creation via the Cell() constructor's cell_defaults assignment - acceptable, or worth skipping start() for the throwaway default clone?

Opened 2026-08-07 (2 days before the meeting), authored by drbergman. mergeStateStatus UNSTABLE: 13 failing checks - 12 are PhysiBoSS Cell Lines/Tutorial result-validation failures on every platform (build+run pass, outputs differ), 1 is an unrelated msys2 setup flake on prey-predator Windows; 152 checks pass. Copilot review failed on quota; one drbergman self-review comment explains the operator[] hardening. No …

Related: #362 conflicts-with · #415 conflicts-with · #364 same-subsystem · #403 same-subsystem

Rewrites beta/setup_libroadrunner.py to fetch per-platform roadrunner zips from PhysiCell-Tools/intracellular_libs (replacing 2016-era SourceForge tarballs, including a manylinux build), simplifies the unzip logic to a single path, and creates a backwards-compat symlink roadrunner/include/rr/C -> . because newer C-API bundles dropped the /C header subdirectory. Also changes the libRoadrunner CMakeLists include path from include/rr/C to include/rr, and trims dead comments from the ode_energy Makefile while adding reminder echoes about DYLD/LD_LIBRARY_PATH. Author tested on Windows, mac Intel/Silicon, and Ubuntu 24.04.

• Upgrading over an old install crashes the script: an existing roadrunner dir with the old include/rr/C directory fails the new rrc_api.h check, re-downloads, then os.remove('roadrunner/include/rr/C') raises IsADirectoryError (only FileNotFoundError is caught). Users must know to delete addons/libRoadrunner/roadrunner manually; catching OSError or using shutil.rmtree would fix it
• os.symlink on native Windows requires admin rights or Developer Mode; if it fails the error is swallowed (printed) and the Windows Makefile's -I...include\rr\C path then points at nothing. A copy fallback would be more robust - was the Windows test under MSYS2?
• CMakeLists inconsistency: the include path changed to include/rr, but three of four platform URLs still fetch the old SourceForge archives whose headers live in include/rr/C - the CMake libRoadrunner build is plausibly broken on Linux/Windows/mac-Intel, and no CI job exercises it so green checks prove nothing here (#415 carries this same hunk)
• Cosmetic: the final print('Done.') is indented inside the except OSError block, so it only prints when symlink creation fails; mac_silicon variable is now set but unused

• Kills dependence on decade-old SourceForge binaries (Python 3.5/3.6-era builds) in favor of maintained, minimal zips under project control
• Substantial simplification: one zip-extract path instead of per-OS tar/zip/rename branching (+43/-76 in the script)

• Merge this before #415, which vendors these exact changes and explicitly depends on it (and on #362)
• Ask rheiland to catch OSError (or rmtree) around the include/rr/C handling so upgrading over an old install doesn't traceback, and to fall back to copying headers when os.symlink fails on Windows
• Decide whether to also update the CMakeLists SourceForge URLs to the new intracellular_libs zips now - the include-path change without the URL change likely breaks the (untested) CMake libRoadrunner build on 3 platforms; consider adding a CI job that builds ode_energy
• Confirm on a native Windows (non-MSYS2, non-admin) machine that the symlink is actually created, since the Windows Makefile still requires include\rr\C

Opened 2026-02-09 (~6 months). MERGEABLE/CLEAN, all 165 CI checks pass, but no reviews and no comments; note CI never compiles the libRoadrunner addon, so the passing checks don't cover this code path. #415 (vincent-noel) already builds on top of it.

Related: #415 should-merge-before · #425 same-subsystem · #362 same-subsystem

Extracts the intracellular block of Cell_Container::update_all_cells into a new public method update_all_cells_intracellular(). Instead of each cell running pre_update -> update -> post_update back-to-back inside one parallel loop, it now runs three separate parallel loops with implicit barriers: first every ready cell's pre_update_intracellular, then every ready cell's intracellular->update(), then every post_update_intracellular. This guarantees a cell's pre-update never reads values another cell's update/post-update already wrote this step (e.g. surface-bound ligand expression). Readiness (need_update()) is cached in a per-cell flag vector, an OpenMP ||-reduction detects whether any cell needs updating, and the function returns early when none do. The helper reads the global diffusion_dt instead of the dt argument, and the caller keeps the existing 'initialzed' first-step gate.

• Data race: ready_to_update_intracellular is a std::vector<bool> written concurrently from the parallel loop (ready_to_update_intracellular[i] = true). vector<bool> packs bits, so writes to different indices hit the same word; this is UB and can lose 'ready' flags under contention. Should be std::vector<char>/<int> or std::deque<bool>. Ironically Copilot flagged the scalar flag (fixed with a …
• The helper hard-codes the global diffusion_dt instead of the diffusion_dt_ argument of update_all_cells; anyone calling the 4-arg overload with a non-global dt gets silently different intracellular stepping (author notes this deliberately in commit messages, but it changes the meaning of the public API argument).

• Fixes a real, subtle ordering bug for models using both pre- and post-update hooks; the three-phase structure is the correct fix
• Early return when no cell needs an intracellular update avoids two pointless parallel sweeps

• Author (drbergman) should swap std::vector<bool> for std::vector<char> (or similar) to remove the concurrent-write UB before merging
• Decide merge order among #363, #364, #427 and #415 - all four rewrite the same region of update_all_cells; #363 and #364 are both drbergman's and trivially combinable (363 keeps the 'initialzed' gate that 364 deletes), so consider merging them as a pair or squashing into one PR
• #415 (multi-intracellular) keeps the old single-pass per-cell structure; if the team wants #415's architecture, the three-phase ordering from this PR must be re-implemented inside it - agree on who does that
• Confirm the intentional switch from the diffusion_dt_ argument to the global diffusion_dt inside the helper is acceptable (affects anyone driving update_all_cells with custom dts)

Opened 2025-02-18, ~18 months old, last updated 2025-05-22. MERGEABLE/CLEAN. No CI runs recorded (predates or expired). Only reviews: Copilot (1 comment, race on the scalar flag - addressed via reduction) and a drbergman self-reply. No human reviewer has looked at it.

Related: #364 conflicts-with · #415 conflicts-with · #427 same-subsystem

Removes the Cell_Container::initialzed flag entirely. Previously the intracellular update was skipped on the very first diffusion step (cells had to wait one step), while phenotype and mechanics were force-run at t=0 via !initialzed special-casing. This PR replaces the 't - last_*_time' bookkeeping with function-local static accumulators (time_since_last_phenotype/mechanics) initialized to phenotype_dt/mechanics_dt so the first step still triggers phenotype and mechanics, and the intracellular loop now runs unconditionally from step one. Also does cosmetic cleanup: precomputed time_for_phenotype/time_for_mechanics booleans, braces added, dead comments deleted.

• Function-local statics are shared across all Cell_Container instances and never reset: a second simulation in the same process (parameter sweeps in-process, library embedding, unit tests) starts with stale accumulators, unlike the old per-instance members. The old member 'initialzed' was reset with each new container.
• Thresholds are frozen from the first call's arguments (static ... = phenotype_dt_ - 0.5*diffusion_dt_), so callers passing different dts on later calls are silently ignored
• last_cell_cycle_time and last_mechanics_time members are left declared in the header but now unused - dead state that should be removed
• Timing now accrues by repeated += diffusion_dt_ instead of differencing absolute t; drift is negligible in practice but it is a semantics change worth one reviewer glance

• Removes the (misspelled) initialzed flag and three special-case branches; the resulting logic is genuinely easier to follow
• First-step intracellular update is arguably the correct behavior; the one-step delay had no documented rationale

• Get an explicit yes/no from vincent-noel (PhysiBoSS) and the dFBA team on whether skipping the first-step intracellular update was ever intentional - the PR body asks exactly this and has waited 18 months
• Decide whether function-local statics are acceptable or whether the accumulators should stay as Cell_Container members (reset-able, per-instance); this matters for anyone running several simulations in one process
• If accepted, PhysiBoSS golden test outputs will likely shift (first boolean update moves one step earlier) - plan to regenerate validation references
• Remove the now-dead last_cell_cycle_time/last_mechanics_time members

Opened 2025-02-18, ~18 months stale, MERGEABLE/CLEAN, labeled low priority by the author. No CI results recorded. Only Copilot reviewed (no comments); zero human review activity. Author is drbergman himself.

Related: #363 conflicts-with · #415 conflicts-with · #427 same-subsystem · #425 same-subsystem

max_cell_interactive_distance_in_voxel is a never-reset per-voxel high-water mark used by is_neighbor_voxel() to decide which neighboring voxels to scan for mechanical interactions. It was previously only raised in assign_position(), set_total_volume(), and convert_to_cell_definition() — never when a cell simply migrated into a new voxel — so a large-interaction-radius cell moving into a 'quiet' voxel could be invisible to neighbor searches, producing the stale/asymmetric neighbor lists discussed in PR #397. This PR adds the raise inside Cell::update_voxel_in_container(), inside the branch that fires only when the mechanics voxel index actually changed (exactly what drbergman suggested in his comment). Since the value only ratchets upward, the fix can only add voxel scans, never remove them. It also carries an unrelated hitchhiker hunk in modules/PhysiCell_settings.cpp changing the mkdir guard from __MINGW32__/__MINGW64__ to _WIN32 (shared with #410 — both branched from heberlr's fork).

• update_voxel_in_container() is also reachable from Cell::fuse(), which runs inside the parallel standard_cell_cell_interactions loop, so the new read-compare-write can race there (worst case: a lost max update, i.e. the original bug transiently persists). The main call sites (the serial 'update cell indices' loop in update_all_cells and divide()) are serial and safe. #427 wraps the identical code …
• Per vincent-noel's comment, this does NOT fully resolve the neighbor-list inconsistencies from #397 — remaining asymmetries may be out-of-domain cells or non-movable cells (#410)
• Unrelated _WIN32 hunk in modules/PhysiCell_settings.cpp is harmless (that branch is dead when __cpp_lib_filesystem exists) but doesn't belong in this PR

• Root-cause fix in the right place: only runs when the voxel actually changes, negligible cost
• Fail-safe by construction — the high-water mark only grows, so it can only widen neighbor searches

• Decide merge order vs #427: merging #409 first gives heberlr credit for the focused fix and forces a trivial rebase of #427; merging #427 first makes #409 redundant and it should be closed with thanks
• If #409 merges standalone, decide whether to add the omp critical that #427 uses (the Cell::fuse() call path runs in a parallel loop)
• Ask heberlr to drop (or the team to knowingly accept) the unrelated _WIN32 mkdir hunk in modules/PhysiCell_settings.cpp, which also rides along in #410
• Keep the #397 neighbor-inconsistency investigation open — vincent-noel confirmed this fix alone doesn't eliminate his inconsistencies; drbergman suggested testing #409+#410 together and checking whether the asymmetric cells are out-of-domain

Opened 2026-02-16 by heberlr. MERGEABLE, mergeStateStatus UNSTABLE. Active discussion: drbergman suggested moving the code into update_voxel_in_container (done, thumbs-up); vincent-noel reports it doesn't fully fix his #397 inconsistencies; heberlr asked for a repro model — thread unresolved. No formal reviews. Behavior flag: neighbor lists become more complete, so results change wherever the stale-max bug …

Related: #427 superseded-by · #410 complements · #397 related

Removes is_movable from the update-velocity loop gate in Cell_Container::update_all_cells so non-movable cells also run standard_update_cell_velocity (building their own state.neighbors and accumulating state.simple_pressure), and compensates inside Cell::add_potentials by early-returning before the velocity axpy when is_movable==false. I verified the early return sits after both the simple_pressure accumulation and the neighbors.push_back, so pressure and neighbor tracking work as intended while pairwise forces are skipped. However, the position-update loop still skips non-movable cells, and update_position is the only place velocity gets zeroed — and standard_update_cell_velocity still adds the motility vector (and any basement-membrane forces) to velocity unconditionally. Also carries the same unrelated _WIN32 mkdir hunk as #409.

• PR body claims velocity is 'explicitly set to zero' for non-movable cells, but no code does this. For a motile non-movable cell, velocity += motility_vector accumulates every mechanics step and is never reset (update_position, which zeroes velocity, still skips non-movable cells). If the model later flips is_movable back to true — a common freeze/unfreeze trick — the Adams-Bashforth step applies …
• Reproducibility: update_motility_vector consumes RNG draws for every motile non-movable cell, shifting the global RNG stream and changing results of all other cells — collides philosophically with #427's thread-reproducibility work
• Performance regression: is_movable=false has long been the cheap way to add many static agents; each such cell now does a full Moore-neighborhood scan every mechanics step. Could be significant for models with large static populations — worth a benchmark or an opt-in flag
• add_cell_basement_membrane_interactions (if set) also still writes into the non-movable cell's velocity before add_potentials — same never-reset accumulation path

• Addresses a real asymmetry: non-movable cells appeared in movable cells' neighbor lists but had empty lists and zero pressure themselves, breaking 'contact with X' and 'pressure' signals/rules for static cells
• The early-return placement in add_potentials is exactly right — pressure and neighbor recording happen, force accumulation doesn't

• Ask heberlr to actually zero velocity (or skip update_motility_vector) for non-movable cells — the PR body promises this but the code doesn't do it; decide the intended semantics of a 'motile but non-movable' cell
• Decide whether non-movable cells participating in full neighbor scans should be default-on or opt-in (performance for models using is_movable=false as an optimization for large static populations); request a benchmark on a static-heavy model
• Coordinate with #427: the velocity-update loop this PR edits is duplicated in #427's counter-based-RNG 'ordered' phase functions — whichever merges second must apply the is_movable change in both places
• Note the behavior change: pressure and contact signals for non-movable cells go from 0 to real values — any published model with rules on those signals for static cells changes behavior

Opened 2026-02-17 by heberlr, head commit 2026-05-08 (~3 months stale). MERGEABLE, mergeStateStatus UNSTABLE. No comments or reviews on the PR itself, but discussed in #409's thread where drbergman suggested testing #409+#410 together against vincent-noel's inconsistencies — that test appears not to have happened.

Related: #409 complements · #427 conflicts-with · #397 related

The signal dictionary registers five built-in contact signals after the n per-cell-type ones (live, dead, apoptotic, necrotic, other dead), but three code paths in core/PhysiCell_signal_behavior.cpp still assumed only two: (1) get_cell_contact_signals rescaled only the first n+2 entries, leaving apoptotic/necrotic/other-dead unscaled; (2) get_selected_signals routed indices beyond n+2 to get_single_signal instead of the precomputed contact vector; (3) get_single_signal's gate excluded the last three, so asking for 'contact with apoptotic cell' fell through to the unknown-signal warning and returned 0. The PR introduces int contact_with_built_in = 5 and uses it at all three sites. I verified against the head that the dictionary indices are consecutive, get_cell_contact_signals fills output[n+2..n+4] correctly, and get_single_signal's internal per-signal branches (which already existed) are now reachable. This directly implements drbergman's CHANGES_REQUESTED review, which identified sites (1) and (2) on top of the author's original fix to (3).

• Style nit: contact_with_built_in is a mutable, external-linkage global int in the .cpp — should be static const int (or constexpr) to prevent accidental mutation and symbol collision; trivial pre-merge tweak
• If a 6th built-in contact signal is ever added to the dictionary, this constant must be bumped in lockstep — consider deriving it or adding a comment next to the dictionary setup

• Fixes a genuine user-facing bug: rules or PhysiBoSS models reading 'contact with apoptotic/necrotic/other dead cell' via get_single_signal got a warning and 0; via get_cell_contact_signals got unscaled values
• All three sites drbergman flagged in review are fixed, using a single named constant as he and the author agreed

• drbergman's Feb 2026 CHANGES_REQUESTED review was fully addressed by elmbeech's 2026-03-21 commit but never re-reviewed — dismiss/refresh the review and approve (stale for ~4.5 months)
• Optionally ask for static const int contact_with_built_in = 5 (or constexpr) before merge
• Flag the behavior change in release notes: 'contact with apoptotic/necrotic/other dead cell' signals now return correctly scaled values where they previously returned 0 (with a warning) or unscaled counts — existing models using them will change behavior (for the better)
• Check whether #385 (also touches PhysiCell_signal_behavior.cpp) merges cleanly around this; regions look disjoint but merge order is worth a 10-second check

Opened 2026-02-18 by elmbeech. MERGEABLE / CLEAN. drbergman reviewed CHANGES_REQUESTED (2026-02-18) identifying two additional buggy sites; author implemented the agreed named-constant approach on 2026-03-21 and pinged — awaiting re-review since. Reviewer sentiment positive ('great catch! Seems like the right fix').

Related: #385 same-subsystem · #423 same-subsystem

Fixes the long-standing #199 bug: Death::models holds pointers to the global apoptosis/necrosis Cycle_Model objects, so the fixed_duration flag (stored in Phase_Link) and death-phase durations (written into models[i]->data by the XML parser) were shared across all cell definitions — last definition parsed won for everyone. The PR (a) removes Phase_Link::fixed_duration and adds Cycle_Data::fixed_durations (vector<vector<char>>, indexed like transition_rates, with fixed_duration(i,j)/exit_fixed_duration(i) accessors), sized in Cycle_Data::sync_to_cycle_model; (b) adds Death::model_data (one Cycle_Data per death model), seeded from the model at add_death_model(), written by the XML parser instead of the shared model, and applied via a new Cycle::sync_to_cycle_model(cm, const Cycle_Data&) overload at both death-entry sites (start_death and the check_for_death branch of advance_bundled_phenotype_functions, the latter being the death_rate path); (c) updates the six standard models to declare defaults through their own Cycle_Data, and display_cell_definitions to report per-definition values. I verified the mechanics at the head: the two-arg sync repoints data.pCycle_Model correctly; the parser's death branch always calls add_death_model before indexing model_data; the remaining one-arg sync call in trigger_death is inside a comment block; and a repo-wide grep finds no surviving code reference to the removed Phase_Link member (only the deprecated LaTeX user guide). Copilot's four review comments (const-correctness, hoisted resizes, per-cell memory) were all addressed in commit 720652b, confirmed in the head diff, with the memory concern answered by author-run measurements.

• Compile break for user projects: Phase_Link::fixed_duration is removed, and the old documented pattern (e.g. `apoptosis.phase_link(0,1).fixed_duration = true;` from the legacy User Guide) will no longer compile — needs a migration note, or a deliberate decision to keep a deprecated member
• Silent behavior change for custom code that mutates the shared death models at runtime: model_data is snapshotted at add_death_model() (during initialize_default_cell_definition), so later direct edits to the global apoptosis/necrosis model's data (rates/durations) are now ignored at death — such code must switch to death.model_data
• display_cell_definitions() indexes pCD->phenotype.death.model_data[k]; hand-rolled custom code that pushes into death.models/rates directly instead of calling add_death_model would leave model_data short and index out of bounds (edge case, arguably already unsupported)
• Intended but real output change: any config with heterogeneous death durations/fixed flags across definitions produces different (now-correct) results; stock sample configs are homogeneous (all fixed_duration=true, 516/86400), so author's regression run matched exactly

• Implements exactly the design MathCancer proposed on issue #199 (migrate the flag from graph structure to param structure)
• Both death-entry code paths covered, including the easy-to-miss death_rate path in advance_bundled_phenotype_functions

• Bless the API break: is removing Phase_Link::fixed_duration outright acceptable for the next release, or should a deprecated no-op member (or compile-time error message) be kept for one cycle? Old custom codes following the legacy User Guide will fail to compile
• Document that runtime edits to the shared apoptosis/necrosis models no longer reach dying cells — custom codes must write pCD->phenotype.death.model_data[k] instead; add to changes.md and the migration notes
• Decide merge order with #385 (extended asym div) and #427 (thread-reproducible RNG): all three edit core/PhysiCell_cell.cpp / phenotype files; #424 is small and 2 days old vs #385's 2000+ lines — merging #424 first is probably cheapest
• Note the crash linkage drbergman added to #199 on 2026-08-06: the shared fixed_duration=false path is a prerequisite for the detach_cell_as_spring use-after-free that #428 locks against — worth walking through whether #424 + #428 together close that chain

Opened 2026-08-07 (2 days old), author drbergman, branch authored with Claude assistance (origin/claude/issue-199-per-definition-death-params). MERGEABLE/CLEAN. Copilot review left 4 comments (2 const-correctness, 1 redundant resize, 1 per-cell memory concern); all addressed in commit 720652b, with the memory point answered by benchmark data. No human reviews yet. Issue #199 itself was closed 2026-08-08.

Related: #385 conflicts-with · #427 same-subsystem · #428 complements · #380 same-subsystem · issue #199 fixes · issue #395 related

Replaces the per-target vector in Asymmetric_Division with a std::map keyed on unordered pairs (type1,type2), so a division event can convert BOTH the parent and daughter cells to any combination of types, each pair with its own probability. Adds a new XML block <extended_asymmetric_division> with <extended_asymmetric_division_probability name1=.. name2=..> (mutually exclusive with the existing standard_asymmetric_division block, enforced with an error+exit), and registers n(n+1)/2 new behaviors 'extended asymmetric division to A and B' (both name orders accepted) so rules can drive the pair probabilities. The old standard XML path still parses but is rewired onto the new pair map, and standard_asymmetric_division_function is renamed to asymmetric_division_function. Adds a configurable <asymmetric_division_probability_tolerance> option (default 1e-12) for the sum-to-1 check, changes throw to cerr+exit(-1) (absorbing #367), enlarges the cells.mat asymmetric_division_probabilities field from n to n(n+1)/2 doubles, and ships a complete extended_asym_div sample project (~1700 of the 2005 added lines). Iteration order for daughter selection is a deterministic std::map walk, deliberately chosen for seed reproducibility.

• Debug leftover: set_behaviors() unconditionally prints 'Warning: set_behaviors is overwriting values for standard asymmetric division...' on every call — will spam stdout in any workflow that bulk-applies behavior vectors per cell per step.
• Bug in get_base_behaviors(Cell*): the extended-asym-div block reads pCell->phenotype... instead of pCD->phenotype..., so it returns the cell's CURRENT probabilities, not the base (cell-definition) values — inconsistent with every other block in that function.
• Aliasing trap: the standard 'asymmetric division to X' behavior slots and the extended pair slots are two views of the same map; in set_behaviors the extended loop runs last and silently overwrites any edit a user made to a standard slot in a get→modify→set roundtrip.
• API break with no deprecation shim: standard_asymmetric_division_function renamed to asymmetric_division_function, Asymmetric_Division::asymmetric_division_probabilities changes type vector→map, and sync_to_cell_definitions()/the double& accessor are removed — user projects touching any of these fail to compile.
• Output format change: cells.mat 'asymmetric_division_probabilities' grows from n to n(n+1)/2 doubles per cell (written for every cell even when asym div is disabled) — downstream readers with fixed layout assumptions (Studio, pcdl, custom MATLAB) need verification, and per-frame file size grows quadratically in cell-type count.

• Genuinely more general model (both daughters re-typeable) with a clean unordered-pair map design; order of name1/name2 never matters anywhere (parser, behaviors, accessors).
• Determinism handled deliberately: std::map with a normalizing comparator makes daughter selection reproducible from the seed alone (aligned with the #427 reproducibility push), and select_daughter_types uses strict < so a zero-probability pair can never win …

• Author must remove or properly gate the unconditional 'Warning: set_behaviors is overwriting...' print and fix get_base_behaviors to read from pCD, not pCell — both are quick, do before merge.
• Decide the standard-vs-extended behavior aliasing policy: keep both slot families as views of one map (current, with silent extended-wins on bulk set), or drop/hide the standard slots when extended is in play.
• Decide whether to keep standard_asymmetric_division_function as a deprecated alias (one-line wrapper) so existing user projects still compile; likewise whether removing the public vector/sync API needs a changelog callout for 1.15.
• Confirm PhysiCell-Studio/pcdl tolerate the cells.mat field growing to n(n+1)/2, and discuss whether to write it only when asymmetric division is enabled (quadratic per-cell output cost for many cell types); drbergman's own comment notes an extra fread block is needed for his fork's resume feature when this merges into dev.

Open since 2025-09-05, actively updated (last push 2026-08-08, tolerance commit added). MERGEABLE against development but mergeStateStatus UNSTABLE (checks pending/failing). No human reviews — only a Copilot review that failed on quota. One comment, from drbergman himself, flagging an extra MultiCellDS-read block needed for his fork's resume feature at merge time. Contribution credited to davidlzhou; PR is by the …

Related: #367 supersedes · #424 conflicts-with · #370 same-subsystem · #427 same-subsystem · #403 same-subsystem · issue #347 partially-addresses

Deletes 6 lines from add_PhysiCell_cells_to_open_xml_pugi_v2 in modules/PhysiCell_MultiCellDS.cpp: the second add_variable_to_labels entry for elapsed_time_in_phase and the second fwrite of the same value. Verified against issue #229 and against current development: the field is indeed emitted twice today (labels at lines 283 and 345; fwrites at lines 804 and 842 of development's file), and the PR head retains exactly one label and one write, so the fix does what it says. However, since this PR was opened, development gained elmbeech's binary reload reader recreate_sim_state() in the same file, which freads the cell record in hardcoded order and explicitly reads the duplicate field — with a comment citing this very PR (development line ~1661). PR 380's head predates that reader (0 hits for recreate_sim_state at the head), and git reports it mergeable with no conflict, so merging as-is would silently desynchronize writer and reader: every reload of a post-merge output file would mis-read all fields after the cycle block.

• Merging as-is breaks recreate_sim_state() (the reload feature merged into development after this PR was opened): the reader freads the duplicate elapsed_time_in_phase at a fixed position and would consume the death.dead field instead, shifting every subsequent read — the PR needs a companion 5-line deletion of that duplicate fread block
• Output format change: cells.mat loses one column, so any downstream tooling that indexes columns by position (rather than by the labels XML) will shift; label-driven readers like pcdl adapt automatically

• Correctly and minimally fixes the confirmed duplication in issue #229
• Labels XML and binary write are removed together, keeping label-driven readers consistent

• Author (drbergman) should rebase onto current development and remove the matching duplicate fread block in recreate_sim_state() (~line 1661, marked with a comment referencing PR #380) in the same PR
• Decide how to handle old-vs-new output files in the reader: recreate_sim_state has no format versioning, so post-merge builds cannot reload pre-merge outputs (and vice versa) — is a version marker or a release-notes warning enough?
• Flag the column removal in changes.md for downstream tools that index cells.mat by position
• CI shows UNSTABLE: one Windows PhysiBoSS-invasion job failed and cancelled the rest — almost certainly unrelated to a 6-line writer deletion; rerun CI to confirm

Opened 2025-07-28 (12 months old), author drbergman. MERGEABLE but mergeStateStatus UNSTABLE (Windows physiboss-tutorial-invasion CI job FAILURE, sibling jobs cancelled; Ubuntu green — looks flaky/infra). No reviews, no comments. Development has since merged a reader that depends on the duplicate this PR removes.

Related: #385 same-subsystem · issue #229 fixes

Vincent Noel found that running a simulation with 0 cells segfaults when writing output, because add_PhysiCell_cells_to_open_xml_pugi_v2 in modules/PhysiCell_MultiCellDS.cpp dereferences (*all_cells)[0] to size the death-rate and custom-data sections of the cell data legend. The original version skipped writing cell files entirely when no cells existed; after heberlr pointed out that Studio/pcdataloader expect the files to exist (even empty), the PR was reworked to always write the files and instead source structural information from cell_defaults. The current 5-line diff replaces (*all_cells)[0] with cell_defaults in the death-rate count and in the four loop bounds over custom scalar/vector variables.

• THE FIX IS INCOMPLETE AT THE CURRENT HEAD (b521628a): inside the legend loops, the bodies still read (*all_cells)[0]->custom_data.variables[j].name/.units (lines 592-593) and (*all_cells)[0]->custom_data.vector_variables[j].name/.units/.value.size() (lines 601-603) of modules/PhysiCell_MultiCellDS.cpp. With 0 cells and ANY custom data defined in cell_defaults (e.g. the template project's 'sample' …
• drbergman's Feb 2025 approval was on commit f06e0180; the branch was later squashed/rebased (Dec 2025) to a single commit — the approved content may have been lost in that rebase, so the approval should not be trusted as covering the current head.
• Vector custom-data legend also needs cell_defaults for value.size(); if left as-is, cell_data_size would be computed from a non-existent cell.

• Correct architectural choice: cell_defaults is the right structure to define output labels independently of the live cell population (drbergman's own review suggestion, adopted by the author)
• Preserves all output files (empty .mat, graph txt files) so PhysiCell Studio / pcdataloader do not break — heberlr's concern was addressed

• Ask vincent-noel (or just push a fixup) to replace the remaining (*all_cells)[0] reads at lines 592-593 and 601-603 with cell_defaults — then this is merge-ready
• Verify whether the Dec 2025 rebase dropped changes that were in the approved commit f06e0180; the existing approval is stale
• Test protocol before merge: run the template project with 0 initial cells (custom data 'sample' present) and confirm output files are written and readable in Studio
• Consider whether a cell dying mid-run down to 0 cells mid-simulation hits the same path (legend is static/once-only, so only a 0-cell FIRST save triggers legend construction — worth confirming)

Opened Jan 2025, rebased Dec 2025 (now 1 commit, 52 commits behind dev but MERGEABLE; mergeStateStatus UNSTABLE = CI pending/failing). Rich history: heberlr warned about downstream tools needing empty files (addressed); MathCancer agreed files should always be written; drbergman requested changes then APPROVED commit f06e0180 in Feb 2025 ('cell_defaults is clearly the right struct'). Current head content does not …

Related: #380 same-subsystem · #385 same-subsystem · #422 same-subsystem · #401 same-subsystem

Two unrelated changes. (1) In sample_projects/episode/main.cpp's between-episode reset loop, adds pCell->lyse_cell() immediately before the existing pCell->die(). (2) In modules/PhysiCell_settings.cpp create_directory(), changes the preprocessor guard from `defined(__MINGW32__)||defined(__MINGW64__)` to `defined(_WIN32)` (one-arg mkdir on Windows). Reading the code at the PR head: die() calls delete_cell(), which already removes attachments, spring attachments, neighbors, and releases internalized substrates before freeing the cell — everything lyse_cell() sets (dead flag, zero volume, null function pointers, is_movable) is destroyed one line later. The one lasting side effect of lyse_cell() is flag_for_removal(), which pushes the cell pointer into the current Cell_Container's cells_ready_to_die list; since die() then frees the cell, that list holds a dangling pointer. It is never dereferenced only because the episode loop happens to allocate a brand-new Cell_Container each episode (create_cell_container_for_microenvironment always news one, leaking the old). The _WIN32 change is a no-op for MinGW (which defines _WIN32) and is forward-prep for MSVC, but note the file only includes <sys/stat.h>, so true MSVC support would additionally need <direct.h>.

• lyse_cell() before die() leaves a dangling pointer in the old Cell_Container's cells_ready_to_die list; harmless today only because the episode loop replaces (and leaks) the container each episode — fragile, and update_all_cells would call die() on freed memory if the container were ever reused
• Everything else lyse_cell() does is undone by the immediate delete in die(), so the functional motivation is unclear from the diff; the PR body ('to reset the cell seeding for the next episode') does not identify the concrete symptom
• Pre-existing (not introduced here): the reset loop range-for iterates (*all_cells) while die()/delete_cell swap-removes from it — it happens to visit every cell via stale tail slots, but relies on unspecified vector behavior; a while(!all_cells->empty()) delete_cell(0) loop would be robust
• The _WIN32 hunk is unrelated to the PR's stated purpose and is duplicated bit-for-bit in elmbeech's PRs #376 and #390 (shared commit)

• Tiny and mergeable CLEAN
• _WIN32 guard is strictly more correct than the MinGW-only check and dovetails with MSVC support work in #403

• Ask elmbeech at the meeting: what concrete misbehavior did lyse_cell()-before-die() fix in the episode workflow? As read, delete_cell already does everything needed, and lyse_cell leaves a stale pointer in the old container's kill list
• Consider instead fixing the reset loop properly (delete-while-range-iterating over all_cells) — e.g. while(!all_cells->empty()) delete_cell(...) — which would be a defensible change for the episode sample
• Decide which of #375/#376/#390 carries the shared _WIN32 mkdir commit, and ask elmbeech to rebase the others so each PR is one logical change
• If merging the _WIN32 guard, note MSVC would still need <direct.h> for mkdir — coordinate with #403 (MSVC support)

Opened 2025-06-13 (14 months old), author elmbeech. MERGEABLE/CLEAN. No reviews, no comments. Shares its settings.cpp commit with #376 and #390.

Related: #376 same-subsystem · #390 same-subsystem · #403 complements · #427 same-subsystem

Adds an opt-in deterministic RNG mode (`<options><rng_mode>counter_based</rng_mode>`): a vendored-style reimplementation of Philox4x32-10 (modules/philox.h) is keyed by (cell_id, time_step, purpose, sub_index), and update_all_cells() installs a thread-local 'deterministic context' around each per-cell phase (12 purpose codes) so existing UniformRandom()/NormalRandom()/etc. call sites transparently draw reproducible per-cell streams without rewriting call sites. To make results bit-identical across thread counts it also sorts cells_ready_to_divide/die by ID before serial processing, and rewrites three phases (secretion, dynamic spring attachments, cell-cell attack/ingest/fuse) as `#pragma omp ordered` loops — but only when the mode is on; legacy runs keep the original fully-parallel loops, deliberately leaving those three races unfixed by default. One fix is unconditional: max_cell_interactive_distance_in_voxel is now updated (under omp critical) when a cell changes mechanics voxel — this adopts PR #409's stale-neighbor-bound fix and changes legacy results too. Ships a thread-reproducibility test harness (beta/test_thread_repro.py) and an unusually thorough 284-line design doc (protocols/counter_based_rng.md).

• Counter construction is non-injective: make_counter() hashes 4x64-bit inputs into a 128-bit counter via xor/splitmix64 (cell_id^(purpose<<1); time_step^(sub_index<<1)^(purpose<<17)), so distinct (cell,step,purpose,sub) tuples can collide and yield identical random streams — e.g. cell c at step s^(1<<17) with purpose p collides with cell c^2 at step s, purpose p^1; step 131072 is t=1310 min at …
• The 'bit-identical regardless of thread count' claim is empirical, not proven: the phenotype loop stays fully parallel, and rules/signals read neighbor state that other threads mutate in the same loop (check_for_death sets phenotype.death.dead immediately; standard_cell_transformations converts type in-loop; 'contact with dead cell' style signals read those). Models using neighbor-state-dependent …
• The unconditional Fix 3 (max_cell_interactive_distance_in_voxel update on voxel change, = PR #409) changes results for ALL users, not just opt-in ones — and CI confirms: the 'worm' and 'virus macrophage' sample checks fail because output no longer matches stored reference results. References need regeneration or the fix needs gating (gating a genuine bug fix would be worse).
• Performance in counter mode: the ordered regions cover the entire loop body, so the secretion phase (runs every diffusion_dt, the hottest per-cell loop in PhysiCell) plus spring-attachment and cell-cell-interaction phases become effectively serial with added DOACROSS sync overhead — likely slower than a plain serial loop. No benchmarks provided; a cheaper deterministic secretion (per-voxel …
• RANDOM_PURPOSE_SPRINGS is reused for both dynamic_spring_attachments and the subsequent spring-force loop, and the force loop resets sub_index per attachment — latent key-stream reuse (harmless today since neither draws randoms, but a footgun for future edits).

• The four races are real: I verified in the code that simulate_secretion_and_uptake() does unlocked read-modify-write on shared voxel densities, dynamic_spring_attachments' capacity check is never re-validated under the attach lock, …
• Design is minimally invasive: context redirection at the utility layer means zero changes to user call sites; off-by-default with legacy loops byte-for-byte preserved.

• Policy decision: is it acceptable to ship v-next with the secretion lost-update race still present in default mode? Alternative: fix races unconditionally with cheap primitives (critical/atomic for secretion — race-free but not bit-deterministic) and reserve `ordered` for counter mode only. PR #428 (drbergman) already takes the unconditional-fix approach for the spring race.
• Decide the ungated Fix 3 fallout: worm and virus-macrophage CI checks fail because default output changed (same change as PR #409). Regenerate reference outputs, and formally close/merge #409 as superseded (note: #427 adds the omp critical that #409 lacks, though the call site is currently a serial loop).
• Merge sequencing: #410 (heberlr, non-movable-cell neighbors) and #427 both rewrite parts of PhysiCell_cell_container.cpp — decide order and who rebases. Also #376 touches PhysiCell_utilities.{h,cpp}.
• Ask author to consider an injective counter packing (cell_id/step/sub as 32-bit fields, purpose in the key with the seed) to eliminate stream-collision risk — small change, removes a whole class of subtle correlation bugs.

Brand new (opened 2026-08-07, 2 days old). MERGEABLE but merge state UNSTABLE: worm and virus-macrophage ubuntu sample checks FAIL at the results-comparison step (expected — the un-gated max-distance fix changes default output vs stored references); all other builds/samples including Windows/macOS pass. Only reviewer activity so far: two inline comments from drbergman (2026-08-07) asking that the default rng_mode …

Related: #409 supersedes · #428 complements · #410 conflicts-with · #376 same-subsystem · #397 same-subsystem

Promotes the function-local 'static bool warned' inside setup_rng() to a global 'bool warned_rng' (declared extern in PhysiCell_utilities.h) so callers can pre-set it and suppress the misleading 'WARNING: Setting the random seed again' message. The episode sample project sets warned_rng = true before reloading the config in episodes 2+, so the warning still fires normally in a first episode if a user genuinely has a legacy random_seed user parameter. Also bundles an unrelated tweak: create_directory() switches its Windows guard from __MINGW32__/__MINGW64__ to _WIN32. Implements exactly what elmbeech and drbergman agreed on in the quoted Slack thread.

• Naming semantics are inverted relative to the Slack proposal (issue_rng_warning=false): callers must set 'warned_rng = true' to mean 'do not warn', i.e. the variable asserts something that has not happened. Works, but reads oddly for an exposed global API.
• The _WIN32 create_directory change is unrelated to the RNG warning and is duplicated verbatim in PR #390 (whichever merges second gets an already-applied hunk — harmless, but the same change riding in two open PRs is messy). Also, under a true MSVC build plain mkdir(path) is not declared (_mkdir is), so _WIN32 is only correct for MinGW-family compilers anyway — relevant if #403's MSVC support …
• warned_rng lives in namespace PhysiCell but is a mutable global with no accessor; fine by codebase conventions, just noting there is no way to re-arm the warning per-episode (not needed today).

• Solves a real, user-reported confusion (collaborators misled by the warning during episode runs)
• Only fires suppression from episode 2 onward, preserving the legitimate first-run warning

• Naming: keep warned_rng, or rename to e.g. suppress_rng_warning to match the Slack design (issue_rng_warning=false) before it becomes public API
• Pick one PR (this or #390) to carry the _WIN32 create_directory change so the same hunk isn't riding in two PRs
• Sequencing vs heberlr's #427 (counter-based RNG): 427 leaves setup_rng's warning untouched but adds ~143 lines to the same file — merge order will require a trivial rebase for whichever lands second; note 427 does NOT subsume this PR
• Confirm the warning suppression should also apply to other episode-style drivers (e.g. PhysiCell Studio runs), or is episode-sample-only fine for now

Opened 2025-06-13 (~14 months old), MERGEABLE/CLEAN. Copilot review, no comments. Design pre-agreed with drbergman on Slack (thread quoted in PR body). No human review recorded.

Related: #427 same-subsystem · #390 complements · #375 same-subsystem · #372 same-subsystem

Adds four Makefile variables (MAGICK_DENSITY := 96, MAGICK_RESIZE_X/Y := 1024, MAGICK_RESIZE) and rewrites the media targets in 20 Makefiles (root Makefile, sample_projects/Makefile-default, all 17 sample projects, and template_BM). The old 8-line 'jpeg' target — which used magick identify + expr + grep + temp files to round the SVG size to even dimensions — becomes a single 'magick mogrify -density $(MAGICK_DENSITY) -format jpg -resize $(MAGICK_RESIZE)' call. The 'gif' target drops the deprecated 'magick convert' subcommand in favor of modern IM7 'magick -density ... -resize ...' syntax, decoupling GIF resolution from simulation domain size. Because jpegs can now have odd dimensions (the old even-rounding is gone), the 'movie' target gains an ffmpeg '-vf pad=ceil(iw/2)*2:ceil(ih/2)*2' filter so libx264/yuv420p never fails on odd sizes; this pad fix is also applied to the two unit_tests Makefiles. All 20 full-change files carry a byte-identical hunk (verified by hashing each per-file diff). Users override resolution via 'make jpeg MAGICK_DENSITY=300 MAGICK_RESIZE_X=2048'.

• Default MAGICK_DENSITY=96 rasterizes SVGs at native size, so for SMALL domains the -resize 1024x1024 step UPSCALES an already-low-res raster — the resulting gif/jpeg is 1024px but blurry. The motivating complaint in #404 ('small domain gives low-res gif') is only truly fixed if the user manually raises density; the issue itself flagged that something 'more robust' (computing density from SVG …
• Behavior change for LARGE domains: jpegs/gifs previously came out at native SVG resolution (e.g. ~1500px for a 1500 um domain); they are now capped at 1024px by default — silently lower-resolution output than before unless overridden.
• Inconsistent sweep: unit_tests/custom_DCs_2substrates and unit_tests/custom_voxel_values Makefiles get only the ffmpeg pad fix; their jpeg target keeps the old temp-file logic and their gif target still uses deprecated 'magick convert'.
• Does NOT address issue #418 (macOS ImageMagick font errors) — nothing here touches font resolution; that issue must stay open.

• Implements exactly the solution the maintainer proposed in issue #404, including the variable names — design already vetted.
• All 20 full-change Makefiles verified byte-identical (md5 of hunks matches), so review reduces to reading one file.

• Decide the default MAGICK_DENSITY: 96 means small-domain gifs are upscaled-blurry (the very complaint that spawned #404). Options: raise the default (e.g. 192/300), use shrink-only resize ('1024x1024>') so small rasters aren't upscaled, or compute density from SVG size in the Makefile — or accept 96 and document the override.
• Confirm the team accepts that large-domain users now get 1024px media by default instead of native resolution (silent quality reduction unless they override MAGICK_RESIZE_X/Y).
• Decide whether to also modernize the jpeg/gif targets in the two unit_tests Makefiles (currently they keep old logic and deprecated 'magick convert') — 5-minute follow-up commit.
• Merge sequencing: #426 (header dependency generation) touches all 22 of the same Makefiles, and #370/#401 touch 15-19 of them; the hunks are in different sections so conflicts should be trivial, but whichever merges second should rebase and re-verify.

Author: drbergman (Copilot coding-agent assisted, per body markers). Created 2026-02-11, last updated same day — ~6 months old but still MERGEABLE / mergeStateStatus CLEAN against development. Zero comments and zero reviews so far, so no reviewer sentiment to report; it has simply never been looked at.

Related: #426 same-subsystem · #370 same-subsystem · #401 same-subsystem · #337 same-subsystem · #357 same-subsystem · #385 same-subsystem · #415 same-subsystem · issue #404 fixes · issue #418 related

Extends copy_file_to_output (core/PhysiCell_utilities.cpp) with an optional default_basename argument: the input file is copied to the output folder under its own basename as before, and additionally under a standardized name (PhysiCell_settings.xml, substrates.csv/substrates.mat, cells.csv, cell_rules.csv) when the names differ. load_PhysiCell_config_file now copies the config XML itself, right after create_output_directory, so config copying moves from user-space main.cpp into core — matching how rules, IC cells, and IC substrates were already handled. The 22 sample-project and unit-test main.cpp files drop their ad-hoc sprintf/system("cp ...") config-copy blocks. The standardized names let downstream tools (e.g. pcdl) reliably find inputs in the output folder. Old user projects that keep their own copy command just copy the file twice — harmless; the visible change is that every output folder now always contains PhysiCell_settings.xml (plus the user-named copy if different).

• Does NOT include PR #371's Windows-pathsep fix: the head still uses find_last_of("/") only, so a Windows-style path (backslashes) yields basename == full path and the first copy silently fails. #371 is therefore NOT subsumed — it's a complementary one-liner in the same function.
• Still built on sprintf + system("cp ..."): silent failure (return ignored), no quoting (paths with spaces break), 1024-byte fixed buffer, and 'cp' doesn't exist under native Windows cmd.exe. Pre-existing, but this PR makes the function the universal copy path, so it's the moment to switch to a plain C++ stream copy (works in C++11, no std::filesystem needed).
• Multiple enabled <ruleset> entries each call copy_file_to_output(..., "cell_rules.csv") in the parse loop — the last enabled ruleset silently overwrites cell_rules.csv, so downstream tools see only one of N rule files.
• Leftover redundant copy_command_2 blocks in sample_projects_intracellular/boolean/tutorial/main.cpp and boolean/cancer_invasion/main.cpp — they now duplicate exactly what core does; should be deleted for consistency with the other 20 mains.
• Pre-existing ordering quirk left in place: the substrate-IC copy inside setup_microenvironment_from_XML runs BEFORE create_output_directory in load_PhysiCell_config_file, so it silently fails for a fresh custom output folder (the new config copy is correctly placed after).

• Guarantees the config file lands in output for every project — previously each user's main.cpp had to remember to do it
• Standardized output filenames (PhysiCell_settings.xml, cells.csv, substrates.csv/.mat, cell_rules.csv) make downstream tooling (pcdl etc.) robust to user-chosen input names

• Confirm #371 is NOT subsumed: decide whether to merge #371 to master and merge forward, or fold its find_last_of("/\\") one-liner directly into this branch (they touch different lines, so both merge cleanly either way).
• Decide whether to replace sprintf/system("cp") with an ifstream/ofstream copy now (C++11-compatible, fixes silent failures, spaces-in-paths, and native-Windows portability) or defer; Copilot flagged this too.
• Multiple enabled rulesets all overwrite output/cell_rules.csv — acceptable, or suffix with an index?
• Merge-order call: this PR shares 13-16 files (mostly the same main.cpp hunks) with #401 (BioFVM Interface) and #413 (mechanics grid XML); whichever merges second will need a rebase — this small mechanical PR should probably go first.

Open ~15 months (created 2025-04-29, last update 2025-05-22); mergeable MERGEABLE / CLEAN against development. No human reviews besides the author's own COMMENTED self-review (confirmed all mains updated; episodes project intentionally untouched). Three Copilot review passes: flagged the undefined-looking 'basename' comparison (false alarm — it is in scope), the system("cp") shell-injection/portability risk, and …

Related: #371 complements · #401 conflicts-with · #413 conflicts-with · #337 same-subsystem · #376 same-subsystem · #427 same-subsystem · #417 same-subsystem · #390 same-subsystem · issue #220 related

Adds a new `<mechanics_voxel_size>` tag inside the `<options>` block, parsed in `PhysiCell_Settings::read_from_pugixml` into a new `PhysiCell_settings.mechanics_voxel_size` member (default 30.0, matching current behavior). If the tag is absent it prints a notice and keeps 30; if the value is <= 0 it warns and keeps the default. All 17 sample projects plus ode_energy get the tag added to their config (set to 30) and their main.cpp changed from `double mechanics_voxel_size = 30;` to passing `PhysiCell_settings.mechanics_voxel_size` into `create_cell_container_for_microenvironment`. The core change is ~30 lines in modules/PhysiCell_settings.{cpp,h}; the other ~2500 added lines are a new `unit_tests/mech_voxel_size/` directory — a manual (not automated) test project with 4 configs (voxel sizes 20/25/30/missing), a copied 323-line Makefile, a leftover backup config, ~800KB of PNGs, and a README explaining why changing the mechanics voxel size changes simulation results (different Moore neighborhoods change pressure and perturb the RNG call sequence, so cell counts diverge). Config parsing happens in `load_PhysiCell_config_file` before the cell container is created, so the ordering is correct.

• Value <= 0 only prints a warning and silently keeps 30 instead of exiting; inconsistent with the adjacent random_seed parsing which exit(-1)s on invalid input — a typo like a negative or zero value would run silently with the wrong voxel size
• Coverage is incomplete: intracellular boolean/fba samples (cancer_invasion, template_BM, tutorial, cancer_metabolism, ecoli), examples/, tests/timing, and the other unit_tests still hard-code `double mechanics_voxel_size = 30` — harmless (behavior unchanged) but inconsistent, and those projects will ignore the new tag
• New unit_tests/mech_voxel_size dir has cruft: PhysiCell_settings-backup.xml leftover, commented-out `simulate_diffusion_decay` in its main.cpp, a fully duplicated 323-line Makefile, and ~800KB of committed PNGs; it is a manual demo project, not an automated test
• Mixed tabs/spaces in the new PhysiCell_settings.cpp hunk (cosmetic); parse block sets the global `PhysiCell_settings.mechanics_voxel_size` from inside the member function, which matches the file's existing (odd) style

• Core change is tiny, correct, and backward compatible: default stays 30 and existing user projects' main.cpp (which still hard-code 30) are untouched
• drbergman's first review point (duplicate definition in PhysiCell_constants) was addressed — the duplicate was removed in later commits

• Merge ordering: #401 (BioFVM Interface) changes create_cell_container_for_microenvironment's signature and rewrites every sample main.cpp, and #372 also touches all sample main.cpp + PhysiCell_settings.cpp. #413 is trivial to rebase, so decide whether it merges first or gets rebased after the big ones.
• asmelko's comment proposes auto-computing the mechanics voxel size from the max interaction distance each mechanics step. Decide: is the XML tag still wanted as an explicit user override (probably yes), and should auto-sizing become a follow-up issue rather than blocking this PR?
• Resolve drbergman's outstanding request to remove the parameter from main.cpp: core/ cannot reference PhysiCell_settings (modules layer), so passing it at the call site in main.cpp may actually be the right layering — decide and dismiss or enforce.
• Decide fate of unit_tests/mech_voxel_size: keep as manual demo, or slim it (drop backup config, duplicated Makefile, ~800KB PNGs, re-enable diffusion in its main.cpp)? The README explanation might belong in docs instead.

MERGEABLE/CLEAN against development. Opened 2026-02-21, last pushed ~2026-03-30 (~4.5 months stale). drbergman left CHANGES_REQUESTED (Feb 21): point 1 (duplicate definition in PhysiCell_constants) was fixed in later commits; point 2 (remove voxel size from main.cpp, use setting inside create_cell_container_for_microenvironment) was not adopted. rheiland replied with a comment-only review; asmelko commented …

Related: #401 conflicts-with · #372 conflicts-with · #409 same-subsystem · #427 related

Rewrites the color-table setup in paint_by_number_cell_coloring() (modules/PhysiCell_pathology.cpp): the hard-coded 13-color cap is replaced with a full-length table sized to the number of cell definitions, with extra types colored by golden-angle HSV generation (previously types >=13 rendered invisible white). Adds an optional <cell_colors> block under <save><SVG> in the config XML mapping cell-definition names to SVG color strings, with partial coverage allowed; user-claimed built-in colors are removed from the automatic pool. Validates both names (must match a cell definition) and color strings (CSS named / #hex / rgb()) and exits with a clear error on mismatch. Settings parsing adds svg_cell_colors_specified and svg_cell_colors_by_name to PhysiCell_Settings, and the default config gains a commented usage example.

• Index-vs-type mismatch: the colors vector is built keyed by cell-definition INDEX (cell_definitions_by_index[i]->name) but looked up with pCell->type, which is the XML ID attribute. If IDs are not exactly 0..N-1 (they are user-supplied, default -1 when absent, and nothing enforces contiguity), a user's named color is applied to the wrong cell type or (guarded) silently falls back to white. The …
• <cell_color> entries with a missing/empty name attribute or empty color value are silently skipped during parsing — a typo like omitting name gives no warning even though the PR's philosophy is fail-fast validation.
• Validation runs at the first SVG write (first call of paint_by_number_cell_coloring), not at config parse time, so a bad name/color kills the run after setup instead of immediately — acceptable but worth noting; exit(-1) also bypasses any cleanup.
• Only paint_by_number_cell_coloring gets the feature; models using other built-in coloring functions (false_cell_coloring_*, etc.) ignore <cell_colors> with no warning that their block is unused.

• Solves two real limitations (invisible >13th cell types; zero user control of colors) with a backward-compatible, opt-in XML block
• Golden-angle HSV generation is deterministic and perceptually well-spaced; hsv_to_svg_color math is correct

• Fix colors lookup to map pCell->type through cell_definition_indices_by_type (or assert IDs are 0..N-1 at validation time) before merge
• Decide whether to warn/error when <cell_colors> is present but the model's coloring function is not paint_by_number_cell_coloring, and whether Studio should adopt/read the same XML block
• Consider warning on skipped <cell_color> entries with empty name/value instead of silent drop
• Output note for the changelog: models with >13 cell types will see previously-white cells become colored in SVGs (visual output change, simulations unaffected)

Opened 2026-05-18, MERGEABLE/UNSTABLE (failing checks are the same repo-wide Windows CI jobs as other PRs). Positive comment from jeanettejohnson ('This looks great to me') asking a clarifying question drbergman answered; Copilot review with summary only. No formal approvals.

Related: #408 complements · #401 conflicts-with · #413 same-subsystem · #427 same-subsystem

In the template sample project's create_cell_types(), moves the three assignments (cell_defaults.functions.update_phenotype = phenotype_function, custom_cell_rule = custom_function, contact_function = contact_function) from after initialize_cell_definitions_from_pugixml() to before it. Because XML-defined cell types are initialized by copying cell_defaults (verified: *pCD = *pParent in PhysiCell_cell.cpp copies .functions), this makes every cell type inherit the three functions instead of only the 'default' type — addressing a classic user gotcha where phenotype_function silently never runs for user-defined types. Also bundles the same _WIN32 create_directory tweak as PR #376 and a cosmetic newline in main.cpp.

• Mechanically correct (inheritance via *pCD = *pParent confirmed), but it is a deliberate design change: with the move, ALL cell types run phenotype_function/custom_function and have a non-NULL contact_function. Bodies are empty in the template so simulation output is unchanged, but once a user fills in phenotype_function it applies to every cell type — the exact opt-out-vs-opt-in question …
• The edit leaves confusing dead code: three lines above the new assignments, the same fields are still explicitly set to NULL (update_phenotype = NULL; custom_cell_rule = NULL; contact_function = NULL) and then immediately overwritten.
• A non-NULL contact_function on every type makes the engine iterate attached cells calling an empty function — negligible cost, but no longer strictly zero.
• Duplicates PR #376's _WIN32 hunk in modules/PhysiCell_settings.cpp (identical change riding in two open PRs).
• Changes only the template project; the other 15+ sample projects keep the old pattern, so the codebase becomes internally inconsistent about the recommended idiom.

• Targets a genuinely common user confusion (issue #265 territory: functions silently not applying to user-defined cell types)
• Matches most users' intuition about what the template's phenotype_function does

• The core decision: should the template teach opt-in (current: functions only on 'default', users explicitly assign per type) or opt-out (this PR: all types inherit, override if needed)? drbergman leaned opt-in with a commented example; elmbeech and possibly rheiland lean opt-out — get rheiland's opinion as elmbeech requested
• If accepted, remove the now-pointless NULL assignments a few lines above and decide whether the other sample projects should be updated for consistency (and whether PhysiCell Studio's generated code assumes the old layout — see issue #265)
• Strip or consolidate the bundled _WIN32 create_directory hunk with PR #376 so it lands exactly once
• elmbeech states these patch PRs are discussion prompts, not blockers — if the team rejects the inheritance change, close explicitly with the rationale in the thread

Opened 2025-09-30, MERGEABLE/UNSTABLE (failing checks are the repo-wide Windows CI jobs plus one flaky asym-division Windows test). Active comment thread: drbergman questioned the design and the branch base; elmbeech explained he branches from the latest release tag and frames these PRs as discussion points for the next release. No formal reviews.

Related: #376 complements · #372 same-subsystem · #370 same-subsystem · #413 same-subsystem · #408 same-subsystem · issue #265 related

Edits the two GitHub Actions workflows (tests.yml, build_binaries.yml). It replaces the long msys2 install list (binutils, lapack, openblas, libxml2, bzip2, python, python-cffi, bison, flex, ...) with a much shorter one (gcc, make, imagemagick, ffmpeg, unzip, zip, git, ca-certificates, plus zstandard where kept), based on what the author learned writing installation manuals. It also upgrades the macOS jobs from gcc-12/13 to gcc-14. Since the PR was opened (Feb 2025), development has independently moved to gcc@14 and macos-15-intel, so the compiler half is already done upstream; only the msys2 cleanup remains novel, and the PR is now in merge conflict.

• tests.yml Windows jobs run 'python beta/test_run_sample.py' inside the msys2 shell, but the slimmed list drops mingw-w64-x86_64-python entirely (old list had it); unless python arrives transitively, every Windows test job's run step should fail with command-not-found. build_binaries.yml keeps python-zstandard (which pulls python) so it is safe there.
• Dropped libxml2/lapack/openblas/bison/flex are plausibly dead weight from the libRoadrunner/MaBoSS-from-source era, but PhysiBoSS Windows jobs link a prebuilt libMaBoSS whose link-time deps were never re-verified; no CI run is recorded on this PR to confirm.
• Adds imagemagick and ffmpeg to CI installs even though no CI step uses them (they are user-manual conveniences), lengthening every Windows CI run.
• PR is CONFLICTING against development because the same workflow lines (OS matrix, compilers) were updated upstream after it was opened.

• Legitimate cleanup goal: the old msys2 list is crufty and undocumented
• The gcc-14 half of the PR was validated by development adopting the same change independently

• Decide whether the msys2 cleanup is still wanted; if yes, ask elmbeech to rebase onto current development, restore mingw-w64-x86_64-python (or python-zstandard) in tests.yml, and let CI validate on all Windows jobs
• Decide whether imagemagick/ffmpeg belong in CI installs at all, or only in user-facing install docs
• Note the currently failing 'windows (...)' build_binaries jobs on every open PR — worth asking whether a slimmed/updated msys2 list is actually the fix for that repo-wide breakage
• If the cleanup is not wanted, close this PR explicitly rather than leaving it to rot (18 months old)

Opened 2025-02-03 (18 months old), CONFLICTING/DIRTY vs development. Only a Copilot review (no comments generated). No recorded CI runs. The gcc-14/macos runner changes it proposed have since landed on development via other commits, leaving only the msys2 list as live content.

Related: #403 same-subsystem · #337 same-subsystem

Adds a 'start and stop' add-on (addons/start_and_stop) that dumps the entire simulation state to six hardcoded text files (cell_data.txt, bool_data.txt, microenv_data.txt, cells.csv, global_param.txt, random_counters.txt) at the end of a run, and restores it on the next run when a new `start_stop` user parameter is true. To do this it adds hand-written line-oriented operator<</operator>> pairs to essentially every core class: Phase, Phase_Link, Cycle, Death, Volume, Geometry, Mechanics, Motility, Secretion, Molecular, Cell_Interactions, Cell_Transformations, Custom_Cell_Data, Cell_State, Cell_Parameters, Cell_Container, and the full BioFVM Microenvironment including internal Thomas-solver vectors (+862 lines in BioFVM alone). Restart works by re-running setup_tissue() and then sequentially overwriting each cell's state from the file, plus 'replaying' the RNG via draw counters. It also adds 7 pure virtual methods to the Intracellular interface (implemented only for MaBoSS), wires the pattern into 4 sample mains, resurrects the full spheroid_TNF_model sample project (~3k lines), and adds a CI smoke test. Crucially, ~97,500 of the 105k added lines are committed simulation output dumps in start_and_stop_saving_files/, and the PR accidentally replaces the repo's default config/PhysiCell_settings.xml with the TNF model's config.

• API/build break: 7 new PURE virtual methods added to core Intracellular base class (get_number_of_nodes, save_current_nodes, save_current_parameters, read_current_parameter, save_current_parameters_maboss, read_current_parameter_maboss, reinit_maboss) are implemented only in addons/PhysiBoSS/src/maboss_intracellular.h — dFBAIntracellular and RoadRunnerIntracellular become abstract, so …
• RNG replay is broken: counter_double_random/counter_int_random/counter_normal_random in core/PhysiCell_utilities.cpp are declared but NEVER incremented anywhere at the PR head, so save_counters always writes 0 and set_counter_*() would infinite-loop for any nonzero value (while(counter != value) UniformRandom(); with counter never changing). Also plain global ints + thread_local mt19937 …
• Not lossless: no setprecision anywhere, so every double is serialized at default 6 significant digits — a restarted run cannot bit-match a continuous run, contradicting the PR's 'without losing information' claim
• State gaps: Cell_State serialization omits attached_cells, spring_attachments, neighbors, velocities; MaBoSS engine's internal RNG state is not saved; restart only works if setup_tissue() recreates exactly the same number and order of cells as at save time (sequential overwrite with no ID matching)
• Code pasted inside the license /* */ comment block at the top of core/PhysiCell_phenotype.cpp — dead duplicate of read_number_in_line helpers, mangling the license header (real definitions duplicated at line ~2527)

• Do not attempt a line-by-line review at the party — decide strategy only: is a bespoke parallel text-serialization layer the right architecture, or should checkpoint/restart be built on the existing MultiCellDS output files (possibly extended with the missing solver/RNG/boolean state), or a versioned binary format? Every hand-matched operator<</operator>> pair is a permanent maintenance tax on …
• Decide the relationship with PR #427 (heberlr's counter-based, thread-reproducible RNG): #337's RNG counters are non-functional dead code, and #427 provides exactly the primitive a correct restart needs. Restart should probably be rebased on top of #427's mechanism.
• Decide the Intracellular interface question together with PR #415 (vincent-noel's combined intracellular models): if save/restore hooks are added to the base class they must be non-pure virtuals with default no-op implementations and generic (non-MaBoSS-specific) names, or the dFBA/libRoadrunner addons break.
• If the team wants to salvage this, ask the author for a split: (1) remove the ~97.5k lines of committed run outputs, config/initial.tsv, and the accidental replacement of config/PhysiCell_settings.xml; (2) spin the spheroid_tnf_model sample project + make target into its own PR (that part is close to mergeable and coordinates with vincent-noel/PhysiBoSS PRs #403/#415); (3) resubmit the checkpoint …

Opened 2024-11-22 (~15 months old), last updated 2025-05-15 (~3 months stale). mergeable=CONFLICTING / mergeStateStatus=DIRTY against development. Zero comments and zero reviews on GitHub — no reviewer sentiment exists yet; the party is effectively the first review. Author is external (Danariki99 / Riccardo Smeriglio), presumably PhysiBoSS-adjacent given the TNF model content. Headline size (105k additions, 72 …

Related: #427 supersedes · #415 conflicts-with · #403 same-subsystem · #401 conflicts-with · #409 same-subsystem · #362 same-subsystem · issue #55 partially-addresses

Introduces two abstract interfaces, Microenvironment_Interface (~40 pure virtuals) and Basic_Agent_Interface (~45 pure virtuals), plus a BioFVM_implementation singleton factory (get_microenvironment / create_basic_agent / get_all_basic_agents) with a legacy_implementation wrapping today's BioFVM; the implementation is currently hard-coded in load_PhysiCell_config_file via set_instance(new legacy_implementation()). Cell is restructured as Cell : Basic_Agent_PIMPL : Basic_Agent_Interface, where the PIMPL wrapper delegates every agent-state access (position, velocity, volume, rates, secretion) through a pImpl pointer to a backend agent created by the factory. Data ownership is inverted: the secretion/uptake/internalized-substrate vectors are now owned by the agent (or by Cell_Definition), and Phenotype's Secretion/Molecular become views exposing double* accessor methods instead of std::vector members, eliminating the old error-prone pointer-swap synchronization between Basic_Agent and Phenotype. Microenvironment XML setup (setup_microenvironment_from_XML, ~360 lines) moves from modules/PhysiCell_settings.cpp into BioFVM/BioFVM_microenvironment.cpp, making BioFVM depend on pugixml and default_microenvironment_options. All 15+ sample projects, examples, addons (PhysiMeSS, PhysiBoSS, libRoadrunner, dFBA) and Makefiles are updated for the renamed accessors (pCell->type -> get_type(), secretion_rates[i] -> secretion_rates()[i]). This is explicitly a staging PR: the follow-up PR adds the author's highly optimized BioFVM implementation.

• Confirmed ~1.4x runtime regression vs development (rheiland, measured twice on cancer-immune-sample, M1/4 threads, before and after rebase). Root cause is visible in the diff: hot mechanics loops (Cell::add_potentials, update_position) now call get_position()/get_velocity() which are two chained virtual calls (PIMPL -> pImpl) replacing what was a direct data-member access; same for per-cell …
• Type-punning UB: all_cells = (std::vector<Cell*>*) get_all_basic_agents(), and legacy get_all_basic_agents() returns (std::vector<Basic_Agent_Interface*>*)&all_basic_agents. The vector typed std::vector<Basic_Agent*> actually stores Cell* pointers, and Cell no longer derives from Basic_Agent at all. It works only because both share Basic_Agent_Interface as primary base at offset 0; BioFVM's …
• Latent null-deref: Cell_Definition copy constructor / operator= assign phenotype before any sync_to_cell_definition, so Secretion::operator= writes through secretion_rates() which returns nullptr when pCell and pCD are both null. The XML path is safe (sync_to_microenvironment is called first), but classic user-project style 'Cell_Definition my_def = cell_defaults;' segfaults at runtime.
• Secretion::sync_to_current_microenvironment's else-branch dereferences get_microenvironment_i() precisely in the case where no microenvironment exists (previously it gracefully resized to 0).
• Microenvironment_Interface::simulate_time_step() runs simulate_cell_sources_and_sinks, while Cell_Container::update_all_cells also advances secretion per cell; sample projects avoid double-application by calling simulate_diffusion_decay directly, but any new-implementation main using simulate_time_step + update_all_cells would apply secretion twice.

• Performance gate: the 1.4x slowdown (rheiland, confirmed twice) must be resolved. The likely culprit is double virtual dispatch on get_position()/get_velocity() inside add_potentials and other per-neighbor hot loops, which replaced direct member access. Decide an acceptance criterion (e.g. <5% regression on legacy path across 2-3 sample projects) and ask asmelko for a profile.
• Decide the abstraction boundary: per-agent, per-call virtual interfaces (current design) vs a coarse solver-level interface (batch diffusion solve + batch sources/sinks over plain agent data). A batch/SoA-friendly boundary would suit the optimized backend better AND keep the legacy path zero-cost; alternatively compile-time backend selection (template/CRTP or build flag) avoids virtual overhead …
• API-break policy: pCell->position/type/velocity and phenotype.secretion.secretion_rates member access break every existing user project, tutorial, and PhysiCell Studio template. If accepted, this is a PhysiCell 2.0-scale change: decide version number, migration guide, deprecation shims (e.g. keep vector-returning accessors), and Studio coordination.
• Merge-order strategy: land the queue of small core bug-fix PRs first (397, 409, 410, 411, 421, 423, 425, 428, 387, 292, 341, ...), then have asmelko rebase 401 on top; alternatively park 401 on a long-lived 2.0/biofvm-interface feature branch so development stays mergeable for everyone else. Merging 401 first would invalidate essentially every other open core PR.

MERGEABLE / mergeStateStatus CLEAN as of last fetch, but head last updated 2026-02-09 (~6 months stale). No approvals and no changes-requested; drbergman left comment-only reviews (Dec 2025) asking for a runtime-selectable implementation, which asmelko delivered Jan 15 (factory in BioFVM_implementation.h, legacy impl hard-coded in load_PhysiCell_config_file) while also reverting most sample-project churn. rheiland …

Related: #413 conflicts-with · #370 conflicts-with · #385 conflicts-with · #427 conflicts-with · #415 conflicts-with · #357 conflicts-with · #428 should-merge-before · #422 should-merge-before · #424 should-merge-before · #423 should-merge-before · #397 should-merge-before · #409 should-merge-before · #410 should-merge-before · #411 should-merge-before · #387 should-merge-before · #388 should-merge-before · #292 should-merge-before · #341 should-merge-before · #364 should-merge-before · #363 should-merge-before · #407 complements · issue #82 related

Introduces core/PhysiCell_rules_extended.{h,cpp} (~2.1k lines), an OO rewrite of the rules engine: elementary signals with pluggable transformers (partial_hill, hill, linear, heaviside, identity), optional signal reference values (e.g. 'LOW oxygen from 10'), 9 built-in aggregators plus custom (enabling AND-logic via product, min/max, etc.), 3 mediators between decreasing/increasing branches plus custom hooks settable from custom.cpp, arbitrary nesting of signals, and new behavior rule types 'accumulator'/'attenuator' that evolve a behavior via exponential ODE dynamics toward base/saturation (hysteresis) instead of setting it directly. Rules can now be written in a new XML format (<behavior_rulesets>); CSV v3 files are still read through the new engine and default settings reproduce classic multivariate-Hill decreasing-dominant semantics for live cells. Critically, every Makefile swaps PhysiCell_rules.o for PhysiCell_rules_extended.o, orphaning the classic engine (author confirms 'PhysiCell_rules.cpp would be deprecated under this PR'), and every sample's custom.cpp changes setup_cell_rules() to setup_behavior_rules(). Also renames the primary behavior 'transform to X' to 'transition to X' (old name kept as synonym), adds two new sample projects (template_xml_rules, template_xml_rules_extended with an excellent 454-line README), and changes run outputs: rules.txt/detailed_rules.{txt,html} are no longer written, dictionaries.txt loses synonyms, and cell_rules_parsed.csv gains annotated/extended syntax.

• API/link break: setup_cell_rules() no longer exists in any compiled object and PhysiCell_rules.h is no longer included by PhysiCell.h, so every existing user project (and Studio-generated project) fails to compile until custom.cpp and its Makefile are updated; no back-compat alias is provided
• UniformInShell/UniformInAnnulus are defined only in the now-orphaned PhysiCell_rules.cpp and silently vanish from the build, breaking any user code that calls them (they should move to PhysiCell_utilities)
• Synonym bug reintroduced: BehaviorRuleset::find_behavior compares raw behavior strings, so CSV lines using 'cycle entry' and 'exit from cycle phase 0' create two competing rules instead of one merged multivariate Hill response — exactly the bug PR #423 fixes in the old (now-dead) file; the fix is not ported
• Dead-cell semantic change: the old engine left a behavior at its prior value for dead cells when no signal in the rule applies_to_dead; the new engine evaluates those signals as 0 and resets the behavior to its base value every phenotype step, which can change results for existing CSV models relying on dead-cell secretion/volume behaviors
• Lost parse-time validation on the CSV path: the old engine exited with a clear message on unknown signal/behavior names or duplicate signal+response pairs; the new parse_csv_behavior_rule performs none of these checks, deferring typos to per-cell runtime warnings/garbage

• Decide the release vehicle: is dropping the classic engine acceptable in a 1.14.x/1.15 minor release, or is this PhysiCell 2.0 material? Every existing user project and PhysiCell-Studio-generated project breaks at compile time (setup_cell_rules undeclared, PhysiCell_rules.o gone from Makefiles).
• If merging: require back-compat shims — keep setup_cell_rules() as a deprecated alias for setup_behavior_rules() (and possibly apply_ruleset for apply_behavior_ruleset) so old custom.cpp files still build.
• Decide the fate of core/PhysiCell_rules.{h,cpp}: delete them (and drop the ~200 lines of new-but-dead parse_xml_rules code this PR adds there), or keep compiling them alongside; leaving them orphaned in-tree is the worst option.
• UniformInShell/UniformInAnnulus must be relocated to PhysiCell_utilities before merge — coordinate with #379, which fixes UniformInShell in the file this PR orphans (merge #379 first, then port).

CONFLICTING/DIRTY vs development; branched Apr 2025, last commit 2025-08-22 (~1 year stale as of 2026-08-09). No human reviews — only GitHub Copilot (3 minor style comments on the now-dead PhysiCell_rules.cpp code: exit(-1) vs exceptions, a switch(0) oddity); the author replied to all three that the file 'would be deprecated under this PR'. PR body defers to the sample-project README for description and explicitly …

Related: #423 conflicts-with · #379 conflicts-with · #292 should-merge-before · #356 should-merge-before · #385 same-subsystem · #421 same-subsystem · #417 same-subsystem · issue #389 partially-addresses · issue #171 related