fix: Resolve RecursionError crash when deleting multiple armatures #432
No reviewers
Labels
No labels
Added
Avatar Toolkit Idea (Won't be added to Cats)
Cleanup
Fixed
Further Info Needed
In Progress
Might Add
Release Candidate
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Kneelawk/Cats-Blender-Plugin!432
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refs/pull/432/head"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #431
I made heavy use of AI when diagnosing this issue and working on a fix, and am not intimately familiar with the code (new to CATS overall). So a little scrutiny from y'all would be much appreciated~
Summary
This PR resolves a crash in Blender 5.0 where deleting multiple objects with armatures causes infinite recursion in the enum property handling code, leading to
RecursionErrorand complete Blender crashes.Changes
Primary Fix: Remove Direct Property Assignment
60743bbsetattr()attempt in_schedule_enum_fix()that caused recursionpath_resolve()Defense-in-Depth: Add Recursion Guard
d432251wrapped_items_func()Technical Details
Root Cause:
When armatures are deleted, the
Scene.armatureEnumProperty can reference a deleted object. The fix mechanism attempted to update the enum value directly usingsetattr(), which triggered Blender's enum validation, which called the items callback again, creating an infinite loop.The Recursion Chain:
Testing
blender-5x-devbranch)I would appreciate help with testing this change with CATS' various use cases (like making sure this doesn't break something else).
Notes
3e1af6bpartially addressed enum handling but left this recursion path intact60743bb) was enough to prevent my crashes, the second commit (d432251) is precautionary.Related
This comment suggests that there was a reason we were trying to set this directly (for Scene properties), are there any concerns with dropping this (or past context/nuance I'm missing)?
The code looks fine. However, I have two small concerns, the first is a potential performance bottleneck. The current code immediately fixes non-UI contexts, where as your code always schedules a timer, though this should not have much of an impact tbh. The second concern is that multiple rapid deletions might schedule multiple timers, causing a race condition. However, it seems the scheduled_property_set check should handle this issue. I will do some further testing and review the code fully later to ensure nothing breaks.
The performance impact seems negligible (maybe 1 second for larger models). This seems fine, i just have to test more models and will merge later if everything is fine.
This seems fine, not found any glaring issues, merged and mark for the 5.0.2.1 release which will be on friday.