Skip to content

fix: unify filament temp mixing blocking with plate error gating - #589

Open
iesteem wants to merge 10 commits into
mainfrom
fix_top_cover
Open

fix: unify filament temp mixing blocking with plate error gating#589
iesteem wants to merge 10 commits into
mainfrom
fix_top_cover

Conversation

@iesteem

@iesteem iesteem commented Jul 15, 2026

Copy link
Copy Markdown

Summary

Five commits hardening the filament temperature mixing (high/low temp materials on the same plate) blocking path, plus a related defensive fix.

Commit-by-commit

  1. fix: unify filament temp mixing blocking with plate error gating

    • Remove has_sliceable_plate_for_slice_all() hijack in the Preview-tab switch that triggered an unintended "slice all" instead of switching to preview.
    • Add m_filament_temp_blocked flag to PartPlate so can_slice() reflects the filament-temp-mixing state, consistent with m_apply_invalid for object-outside errors.
    • sync_filament_temp_mixing_notification() manages its own flag, independent of the background validation system.
    • Simplify EVT_GLVIEWTOOLBAR_PREVIEW to use can_slice() as a unified gate for both error types.
  2. fix: use on-demand checks instead of cached flag for filament temp mixing

    • Replace the per-plate cached m_filament_temp_blocked flag with on-demand is_plate_blocked_by_filament_temp_mixing() checks in the rendering and preview-switch paths.
    • Eliminates staleness risks identified during code review where non-current plates could retain stale blocked state after filament changes, undo/redo, or plate switching during slicing.
    • GLCanvas3D: on-demand red-exclamation check on plate thumbnails; Plater: on-demand check in EVT_GLVIEWTOOLBAR_PREVIEW.
  3. refactor: simplify filament slot collection in check_filament_temp_mixing

    • Merge duplicate key lists, rename used_slots for clarity, group logic into scoped blocks. No semantic change.
  4. refactor: rename STRING_EXCEPT_FILAMENTS_DIFFERENT_TEMP to STRING_EXCEPT_FILAMENTS_MIXING_TEMP

    • Better reflects that the check is about high/low temperature material mixing, not just "different" temperatures.
    • Touches PrintBase.hpp, Snapmaker_Orca.cpp (CLI), and Plater.cpp.
  5. feat: show per-filament high/low temp breakdown in mixing notifications

    • Plater::check_filament_temp_mixing now exposes a FilamentTempMixingDetail (high/low 1-based slot lists) via a new overload.
    • Notification text becomes dynamic; sync_filament_temp_mixing_notification caches the exact pushed strings to work around NotificationManager's exact-text close matching.
    • Added slice-all variants listing every conflicting plate.

Related defensive fix (bundled)

  • GCodeReader.cpp: parse_file_raw_internal() returns false when fopen fails, preventing fread(NULL) from triggering a CRT invalid-parameter crash.
  • Localization: zh_CN/Snapmaker_Orca_zh_CN.po updated with the new dynamic notification strings (Chinese only; other languages to be synced by CI / update-translation.yml).

Downstream effects

Red exclamation icon on plate thumbnails, disabled slice/export buttons, and blocked auto-slice on preview switch now work uniformly for both object-outside and filament-temp-mixing errors.


Reviewer Focus

Please prioritize the following (ordered by risk):

1. On-demand vs cached flag tradeoff (most important)

Commit 2 reverted the cached flag from commit 1 in favor of on-demand checks. Please confirm:

  • is_plate_blocked_by_filament_temp_mixing() is cheap enough to call on every plate-thumbnail render (GLCanvas3D::_render_imgui_select_plate_toolbar) — it walks model().objects and resolves presets each call. For projects with many plates / objects, is there measurable render-time impact?
  • All read sites that previously trusted m_filament_temp_blocked now go through the on-demand check. Are there any remaining readers of the (now-removed) flag that would silently compile to false?
  • Was the cached flag kept anywhere (e.g. for PartPlate::can_slice())? If can_slice() no longer reflects temp-mixing state, the "unified gating" promise of commit 1 may regress.

2. EVT_GLVIEWTOOLBAR_PREVIEW behavior change

Before: clicking Preview with a sliceable plate auto-fired slice_all.
After: blocking checks run first; if blocked, switch to Preview view without slicing.
Please confirm the non-blocking path still auto-slices as before, and that the workflow change is intentional and surfaced to users (release notes / changelog).

3. Notification close-then-push correctness

sync_filament_temp_mixing_notification() now caches filament_temp_mixing_last_error_text / filament_temp_mixing_last_warning_text for exact-text close. Please check:

  • All early-return paths (e.g. curr_plate == nullptr) — do they leak a stale notification by skipping the close step?
  • Across the three triggers (plate switch, filament change, bed-type change), the previous notification is always cleared before pushing the new one.
  • Slice-all mode caches a different text shape than single-plate mode; verify the close path matches the right cache when switching modes.

4. Slot-collection refactor (commit 3) is purely cosmetic

"No semantic change" is claimed. Please diff-check:

  • The merged key list covers exactly what the two original lists covered (wall_filament, sparse_infill_filament, solid_infill_filament, support_filament, support_interface_filament, wipe_tower_filament).
  • The uses_default_extruder branch + global extruder resolution order is unchanged.
  • The 1-based → 0-based slot conversion is preserved.

5. Enum rename impact

STRING_EXCEPT_FILAMENTS_DIFFERENT_TEMPSTRING_EXCEPT_FILAMENTS_MIXING_TEMP. Search the whole repo (including 3MF serialization, Sentry reporting, CLI) to confirm:

  • No missed call sites.
  • No persistence layer stores this enum by name (only by integer value = 2, which is preserved).

6. Per-filament breakdown formatting

The notification text is now dynamic and lists slots like [1] PLA White; [3] PETG Black. Please confirm:

  • Slot labels match what the user actually sees (preset name resolution, 1-based indexing).
  • Translation still works for the surrounding template — i.e. dynamic slot lists don't break gettext format strings (%1% etc.).
  • For very long slot lists (10+ filaments), does the notification render sanely (no truncation / layout break)?

7. Bundled GCodeReader.cpp fix

  • The fopen NULL check is a defensive fix unrelated to filament temp mixing — confirm it's intentional to bundle here (vs. a separate PR).
  • Does any Sentry crash fingerprint depend on this path crashing? If so, the fix will silence those reports.

8. i18n coverage

Only zh_CN.po is updated; the other 20 languages will fall back to English for the new dynamic strings. Is that acceptable for merge, or should we wait for update-translation.yml to sync?


Test Plan

  • Plate with both high-temp and low-temp filaments in use → ValidateError fires, slice button greyed out, red exclamation on plate thumbnail
  • Click Preview tab on a blocked plate → switches to Preview without auto-slicing; notification remains visible
  • Click Preview tab on a compatible plate → still auto-slices via slice_all (no regression)
  • Resolve the mixing conflict (swap a filament) → notification clears immediately, slice button re-enables, exclamation disappears (no stale state)
  • Undo/redo a filament swap → blocking state tracks correctly (verifies the on-demand check, not a stale cached flag)
  • Switch plates while slicing → non-current plates don't retain stale blocked state
  • Slice All with multiple conflicting plates → notification lists each plate with its high/low slot breakdown
  • Slice All with one blocked / one clean plate → blocked plate skipped, clean plate slices
  • Notification text in zh_CN renders correctly; English / other locales render with English fallback (no crash, no format-string breakage)
  • CLI slice on a mixing-conflict project → still reports the error via CLI_FILAMENTS_DIFFERENT_TEMP (rename is internal-only)
  • Loading a G-code file whose path is invalid → returns cleanly, no CRT crash

🤖 Generated with Claude Code

iesteem and others added 8 commits July 15, 2026 19:11
- Remove has_sliceable_plate_for_slice_all() hijack in Preview tab switch
  that triggered unintended "slice all" instead of switching to preview
- Add m_filament_temp_blocked flag to PartPlate so can_slice() reflects
  filament temp mixing state, consistent with m_apply_invalid for
  object-outside errors
- Let sync_filament_temp_mixing_notification() manage its own flag,
  independent of the background validation system
- Simplify EVT_GLVIEWTOOLBAR_PREVIEW to use can_slice() as unified gate
  for both error types

Downstream effects: red exclamation icon on plate thumbnails, disabled
slice/export buttons, and blocked auto-slice on preview switch now work
uniformly for both object-outside and filament-temp-mixing errors.
…xing

Replace the per-plate cached m_filament_temp_blocked flag with on-demand
is_plate_blocked_by_filament_temp_mixing() checks in the rendering and
preview-switch paths. This eliminates staleness risks identified during
code review where non-current plates could retain stale blocked state
after filament changes, undo/redo, or plate switching during slicing.

Changes:
- GLCanvas3D: add on-demand check for red exclamation on plate thumbnails
- Plater: add on-demand check in EVT_GLVIEWTOOLBAR_PREVIEW alongside
  can_slice(), keep sync_filament_temp_mixing_notification() for
  notification freshness
- PartPlate: revert cached flag; can_slice() unchanged

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…xing

Merge duplicate key lists, rename used_slots for clarity, and group logic
into scoped blocks. No semantic change.
…EPT_FILAMENTS_MIXING_TEMP

Better reflects that the check is about high/low temperature material mixing,
not just "different" temperatures.
Add per-filament high/low temperature breakdown to the filament temp
mixing warning/error notifications so users can see exactly which slots
are in conflict, both for single-plate and slice-all modes.

- Plater::check_filament_temp_mixing now exposes a FilamentTempMixingDetail
  (high/low 1-based slot lists) via a new overload
- Notification text becomes dynamic; sync_filament_temp_mixing_notification
  caches the exact pushed strings to work around NotificationManager's
  exact-text close matching
- confirm_filament_temp_mixing_before_slice / _before_slice_all use the
  same breakdown in their modal dialogs
- Safe translation helper tr_u8() avoids the dangling-pointer trap in
  I18N::translate_utf8() that crashed in non-English locales
- Add zh_CN translations for the new strings

Also fix a separate crash: GCodeReader::parse_file_raw_internal no longer
dereferences a NULL FILE* when fopen fails (e.g. loading a non-existent
gcode file at startup), and Plater::load_gcode now checks wxFileExists
up front to give a precise "does not exist" error instead of the vague
"does not contain valid G-code".
- B1 perf: cache per-plate blocked state in Plater::priv and add
  is_plate_blocked_by_filament_temp_mixing_cached() for the per-frame
  plate-thumbnail render path. Cache is refreshed once per
  sync_filament_temp_mixing_notification() call, which already runs at
  every state-changing event, so hot reads are O(1).
- B2: close previously-pushed mixing notifications BEFORE the
  curr_plate==nullptr early return so the on-screen banner does not
  leak when the current plate is transiently null during a switch.
- B3: remove the tr_u8() helper. I18N::translate_utf8() returns
  std::string by value; the dangling-pointer narrative was incorrect.
  All call sites revert to _u8L().
- B4: warning notification close text mismatch. push_notification stores
  the body verbatim, but close_validate_warning_notification(text)
  compares against "WARNING:\n" + text. Cache the un-prefixed body so
  push/close text match exactly; the previous code never closed the
  warning banner.
- m1: restore Allman brace style in new helpers and check_filament_temp_mixing
  overload to match the rest of Plater.cpp (project coding standard 2.3).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
These two unrelated defensive fixes were bundled into commit 4d7ffd5
alongside the mixing-notification work. Per the change-scope-control
review finding (M3), they have an independent regression surface
(loading missing / invalid G-code files) and should ship in a separate
PR so any regression can be bisected cleanly.

Reverted in this commit (will be re-applied on a separate branch):
- src/libslic3r/GCodeReader.cpp: NULL FILE* check in parse_file_raw_internal
- src/slic3r/GUI/Plater.cpp: wxFileExists early-reject in Plater::load_gcode
- localization/i18n/zh_CN/Snapmaker_Orca_zh_CN.po: "does not exist." key

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@iesteem

iesteem commented Jul 30, 2026

Copy link
Copy Markdown
Author

Review findings addressed

Pushed two commits to fix_top_cover addressing the blocker/major review findings:

  • 1fc3eb9 — B1/B2/B3/B4 + m1 fixes:
    • B1 (perf): added Plater::priv::filament_temp_mixing_blocked_cache + is_plate_blocked_by_filament_temp_mixing_cached(). Cache is refreshed once per sync_filament_temp_mixing_notification() call (which already runs at every state-changing event), so the per-frame plate-thumbnail render path is now O(1).
    • B2 (notification leak): sync_filament_temp_mixing_notification() now closes previously-pushed notifications BEFORE the curr_plate == nullptr early return.
    • B3 (tr_u8): removed. I18N::translate_utf8() returns std::string by value — the dangling-pointer narrative in the helper's comment was incorrect. All call sites revert to _u8L.
    • B4 (warning close mismatch): the warning notification body was being pushed with a WARNING: prefix but close_validate_warning_notification(text) re-prepends the same prefix, so the close never matched. Fixed by caching the un-prefixed body.
    • m1 (style): restored Allman brace style in new helpers and the check_filament_temp_mixing overload to match the rest of Plater.cpp.
  • abe6eb9 — M3 scope split:
    • Reverted GCodeReader.cpp (NULL FILE* check) and Plater::load_gcode (wxFileExists early-reject) to main state, removing them from this PR's diff.
    • These two defensive fixes now live in their own PR: fix: prevent NULL FILE* crash when loading a missing G-code file #648. They have an independent regression surface (loading missing/invalid G-code) and should ship separately for clean bisect.

Remaining items for the author / reviewers

Test plan additions

Please add manual verification for:

  • AllowedWarning state → switch plates → warning banner disappears instantly (verifies B4 fix).
  • Multi-plate project (10+ plates, many objects) → drag 3D view → no frame drops (verifies B1 fix).
  • curr_plate transiently null during plate switch → no stale error banner remains (verifies B2 fix).

iesteem added a commit that referenced this pull request Jul 30, 2026
Two related defensive fixes for the G-code load path:

- GCodeReader::parse_file_raw_internal now returns false when fopen
  fails, preventing ::fread(buffer.data(), 1, ..., NULL) from triggering
  a CRT invalid-parameter crash on Windows (and UB elsewhere).
- Plater::load_gcode rejects a missing / inaccessible file up front
  with a precise "does not exist" message, instead of letting the load
  path proceed and surface the misleading "does not contain valid
  G-code" error.

Originally bundled into PR #589 (filament temp mixing) commit 4d7ffd5;
split out per change-scope-control review finding (M3) so any
regression on the G-code load path can be bisected independently of
the mixing-notification changes.
iesteem added 2 commits July 30, 2026 20:48
The error/warning notification referenced the filament mixing
preference as "Allow mixed printing of high and low temperature
materials", which doesn't match the actual preference label
"Allow high/low temperature filament mixing". Update the
notification source literal and zh_CN translations to use the
canonical name so users can locate the preference the notification
points them to.
iesteem added a commit that referenced this pull request Jul 31, 2026
Annotate is_plate_sliceable() as the single point where GUI-layer
blockers converge, so follow-on PRs (#626 flow_ratio_zero, #589
mixing_cached) know to plug their predicate in here instead of
branching again at GLCanvas3D / MainFrame call sites. No behavior
change; just a comment/contract update so the three concurrent PRs
that all rewrite the same toolbar line can converge without
conflict.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant