Fix/loader regressions 3114 3173 3144 by tay0thman · Pull Request #3179 · pyrevitlabs/pyRevit

tay0thman · 2026-03-16T20:59:35Z

Name of your PR

Description

fix: Dynamo engine defaults, SVG icon fallback, env var expansion (#3114, #3173, #3144)

CommandTypeGenerator: automate defaults to false, clean defaults to false (matches legacy)
IconManager: skip SVG icons in selection and loading (BitmapImage can't render them)
ExtensionParser: expand %VARIABLES% in user extension paths before path resolution

Checklist

Related Issues

Resolves [Bug]: Dynamo Scripts needs to be set to "Automatic" in order to work #3114
Resolves#3173
Resolves [Bug]: Extension Paths with System Variables Fail to be Read by the new Loader #3144

Additional Notes

Include any additional context, screenshots, or considerations for reviewers.

Thank you for contributing to pyRevit! 🎉

…abs#3114) - automate: use EngineConfig.RequiresMainThread ?? false (was ?? true) - clean: use cmd.Engine?.Clean ?? false (was hardcoded true) Matches legacy genericcomps.py lines 522, 610-612.

…bs#3173) - Filter SVG from icon selection via IsRenderable() check - Add early-return guard in LoadBitmapSource for .svg files - Raster icons (png/ico/jpg) are preferred when both exist Legacy loader only ever looked for icon.png.

…s#3144) Add Environment.ExpandEnvironmentVariables() before Path.GetFullPath() in GetExtensionRoots(). Matches legacy os.path.expandvars() behavior. Fixes paths like %%USERPROFILE%%\Documents\MyExtensions.

tay0thman · 2026-03-16T21:00:31Z

@devloai , please review the changes and report.

devloai

Review Summary

Three fixes targeting real bugs (#3114, #3173, #3144). The ExtensionParser env-var expansion and the IconManager SVG-filtering approach are both conceptually correct, but I found two issues worth addressing before merging.

✅ ExtensionParser — `Environment.ExpandEnvironmentVariables` (#3144)

Clean one-liner fix. Exactly the right API for %USERPROFILE%-style paths.

⚠️ IconManager — SVG skip (#3173)

Approach is sound (GetBestIconForSizeWithTheme + IsSvgIcon helper), but there's a null-dereference risk in LoadBitmapSource — see inline comment.

⚠️ CommandTypeGenerator — clean engine / Dynamo defaults (#3114)

The useCleanEngine refactor has a logic issue: after the change both useCleanEngine and explicitlyRequiresCleanEngine are the same expression, making the comment ("Default is true") stale and the else if branch unreachable. Please clarify intent and simplify — see inline comment.

dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs

dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/IconsHandling/IconManager.cs

devloai

PR Summary:

This PR fixes three loader regressions:

CommandTypeGenerator: Changes clean and Dynamo automate engine defaults to false (matching legacy behavior)
IconManager: Skips SVG icons that BitmapImage cannot render, preventing silent failures
ExtensionParser: Expands %ENV_VARIABLES% in user extension paths before path resolution

Review Summary:

The SVG and environment variable fixes are clean and straightforward. However, the CommandTypeGenerator changes introduce two logic issues worth addressing. The LoadBitmapSource SVG guard has a null-dereference risk since it calls imagePath.EndsWith() before the existing null guard. In BuildEngineConfigs, both useCleanEngine and explicitlyRequiresCleanEngine were made identical expressions, making the rocket-mode conditional logic non-functional. Additionally, the Dynamo automate default change (from true to false via RequiresMainThread) silently inverts existing behavior and needs explicit documentation.

Follow-up suggestions:

@devloai fix the identified issues in CommandTypeGenerator.cs (redundant variable logic + stale comment) and the null-dereference in IconManager.cs
@devloai add unit tests for the Dynamo engine config defaults to prevent regressions on the automate/clean default values

dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/IconsHandling/IconManager.cs

dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs

tay0thman

Updated per comments from @devloai

tay0thman · 2026-03-16T21:47:06Z

@devloai Please review, the updates.

devloai · 2026-03-16T21:49:49Z

Thanks for the ping! Here's the updated status after reviewing the latest commit (b2c89ee):

✅ Resolved

IconManager.cs NullReferenceException — null guard is now correctly placed before the SVG check. Fixed.
Rocket-mode dead code — explicitlyRequiresCleanEngine, extensionIsRocketModeCompatible, and the full if/else if block have been removed. Clean.

⚠️ Still Open

1. Stale comment in `CommandTypeGenerator.cs` (line 222)

The comment still says "Default is true (use clean engine for each execution)" but the code now defaults to false. Minor but misleading for future maintainers.

// Determine clean engine setting.
- // Default is true (use clean engine for each execution).
- // In rocket mode with compatible extension, use cached engine (clean = false).
+ // Default is false (metadata-driven; matches legacy cached-engine behavior).
bool useCleanEngine = cmd.Engine?.Clean ?? false;
// No rocket-mode override needed — the logic is now purely metadata-driven

2. Dynamo `RequiresMainThread` default change needs confirmation (line 240)

The old code: (cmd.Engine?.MainThread ?? false) || (cmd.Engine?.Automate ?? true) → default true
The new code: cmd.Engine?.RequiresMainThread ?? false → default false

This silently changes Dynamo automate behavior for scripts without explicit automate: YAML. Could you confirm:

EngineConfig.RequiresMainThread combines both MainThread and Automate YAML fields?
The default of false is intentional per issue [Bug]: Dynamo Scripts needs to be set to "Automatic" in order to work #3114?

Overall the PR is close — just needs the comment fix and confirmation on the Dynamo default semantic change.

tay0thman · 2026-03-16T21:58:59Z

The default of false is intentional per issue [Bug]: Dynamo Scripts needs to be set to "Automatic" in order to work #3114?

@devloai
yes, false is intentional. Here's the evidence chain:
EngineConfig.RequiresMainThread at PyRevitConfig.cs:710 is:
csharppublic bool RequiresMainThread => (MainThread ?? false) || (Automate ?? false);
It combines both YAML fields (mainthread: and automate:) with ?? false fallbacks for both. This already exists in the codebase — the PR just uses it instead of reimplementing the logic with a wrong fallback.

devloai · 2026-03-16T22:00:05Z

Thanks for the detailed evidence chain — that fully satisfies the concern. ✅

Dynamo default (false) — confirmed correct. The legacy Python loader defaults both mainthread and automate to 'false', EngineConfig.RequiresMainThread combines them with ?? false fallbacks, and the old Automate ?? true was the actual bug.

Only remaining nit: the stale comment on CommandTypeGenerator.cs ~line 222 still reads "Default is true (use clean engine for each execution)" while the code now does ?? false. A one-line comment update would make future maintainers' lives easier:

// Default is false (matches legacy cached-engine behavior).
bool useCleanEngine = cmd.Engine?.Clean ?? false;

Otherwise the logic is solid — good catch on all three regressions.

…yrevitlabs#3144

tay0thman · 2026-03-16T23:29:04Z

@devloai Review the latest commits

devloai

Review of latest commits (up to `f21541d`)

✅ Previous Issues — All Resolved

Issue	Status
`IconManager.cs` — null guard ordering (NullReferenceException)	✅ Fixed
Rocket-mode dead code removed	✅ Fixed
Stale comment "Default is true" in `CommandTypeGenerator.cs`	✅ Fixed — comment now correctly says "Default is false"
Dynamo `RequiresMainThread ?? false` default	✅ Confirmed correct (see discussion)

🆕 New commit: `f21541d` — "Pass revitYear to ParseComponents function"

This is a legitimate and necessary fix. Without it, the min_revit_version / max_revit_version component-level gates in ParseComponents (line 1290) were always evaluated with revitYear = 0 (the default), effectively disabling version filtering for all components under ParseExtension. The fix correctly threads revitYear through the call chain:

ParseExtension(revitYear) → ParseComponents(..., revitYear) → IsRevitVersionCompatible(...)

ParseComponents already propagates it recursively to children at line 1127, so the fix is complete. No issues found.

Summary

The PR is ready to merge. All three original regressions (#3114, #3173, #3144) are properly fixed, and the new revitYear propagation fix is a clean and correct addition.

github-actions · 2026-03-17T00:12:15Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26076+0001-wip

github-actions · 2026-03-18T23:29:37Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26077+2319-wip

github-actions · 2026-03-20T23:44:31Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26079+2333-wip

github-actions · 2026-03-21T13:30:02Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26080+1314-wip

github-actions · 2026-03-24T20:42:44Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26083+2031-wip

github-actions · 2026-03-24T20:55:23Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26083+2042-wip

github-actions · 2026-03-24T20:56:08Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26083+2045-wip

github-actions · 2026-03-24T20:58:58Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26083+2048-wip

github-actions · 2026-03-24T21:17:59Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26083+2106-wip

github-actions · 2026-03-24T21:41:13Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26083+2130-wip

github-actions · 2026-03-27T20:15:21Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26086+2004-wip

github-actions · 2026-03-29T13:30:37Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26088+1318-wip

github-actions · 2026-03-30T12:42:36Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26089+1231-wip

github-actions · 2026-03-31T06:00:17Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26090+0549-wip

github-actions · 2026-03-31T15:44:41Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26090+1533-wip

github-actions · 2026-03-31T15:47:30Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26090+1536-wip

github-actions · 2026-03-31T15:51:48Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26090+1540-wip

github-actions · 2026-03-31T15:51:51Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26090+1540-wip

github-actions · 2026-03-31T16:07:36Z

📦 New work-in-progress (wip) builds are available for 6.1.0.26090+1556-wip

github-actions · 2026-03-31T16:14:00Z

📦 New public release are available for 6.2.0.26090+1754

tay0thman added 3 commits March 16, 2026 13:37

fix(loader): Dynamo engine defaults automate/clean to false (pyrevitl…

6a2e924

…abs#3114) - automate: use EngineConfig.RequiresMainThread ?? false (was ?? true) - clean: use cmd.Engine?.Clean ?? false (was hardcoded true) Matches legacy genericcomps.py lines 522, 610-612.

fix(parser): expand env variables in user extension paths (pyrevitlab…

fd8b259

…s#3144) Add Environment.ExpandEnvironmentVariables() before Path.GetFullPath() in GetExtensionRoots(). Matches legacy os.path.expandvars() behavior. Fixes paths like %%USERPROFILE%%\Documents\MyExtensions.

tay0thman requested review from dosymep and romangolev March 16, 2026 21:01

devloai bot reviewed Mar 16, 2026

View reviewed changes

dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs Outdated Show resolved Hide resolved

dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/IconsHandling/IconManager.cs Show resolved Hide resolved

devloai bot reviewed Mar 16, 2026

View reviewed changes

tay0thman commented Mar 16, 2026

View reviewed changes

tay0thman force-pushed the fix/loader-regressions-3114-3173-3144 branch from 1b02407 to b2c89ee Compare March 16, 2026 21:48

build: update compiled DLLs for pyrevitlabs#3114, pyrevitlabs#3173, p…

9d1b93a

…yrevitlabs#3144

tay0thman force-pushed the fix/loader-regressions-3114-3173-3144 branch from b2c89ee to 9d1b93a Compare March 16, 2026 22:00

Pass revitYear to ParseComponents function

f21541d

devloai bot reviewed Mar 16, 2026

View reviewed changes

jmcouffin merged commit f3610d2 into pyrevitlabs:develop Mar 16, 2026

jmcouffin self-assigned this Mar 16, 2026

jmcouffin added the Enhancement Enhancement request [class->Improved #{number}: {title}] label Mar 16, 2026

Uh oh!

Conversation

tay0thman commented Mar 16, 2026

Name of your PR

Description

Checklist

Related Issues

Additional Notes

Uh oh!

tay0thman commented Mar 16, 2026

Uh oh!

devloai bot left a comment

Choose a reason for hiding this comment

Review Summary

✅ ExtensionParser — Environment.ExpandEnvironmentVariables (#3144)

⚠️ IconManager — SVG skip (#3173)

⚠️ CommandTypeGenerator — clean engine / Dynamo defaults (#3114)

Uh oh!

Uh oh!

Uh oh!

devloai bot left a comment

Choose a reason for hiding this comment

PR Summary:

Review Summary:

Follow-up suggestions:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tay0thman left a comment

Choose a reason for hiding this comment

Uh oh!

tay0thman commented Mar 16, 2026

Uh oh!

devloai bot commented Mar 16, 2026

✅ Resolved

⚠️ Still Open

1. Stale comment in CommandTypeGenerator.cs (line 222)

2. Dynamo RequiresMainThread default change needs confirmation (line 240)

Uh oh!

tay0thman commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devloai bot commented Mar 16, 2026

Uh oh!

tay0thman commented Mar 16, 2026

Uh oh!

devloai bot left a comment

Choose a reason for hiding this comment

Review of latest commits (up to f21541d)

✅ Previous Issues — All Resolved

🆕 New commit: f21541d — "Pass revitYear to ParseComponents function"

Summary

Uh oh!

github-actions bot commented Mar 17, 2026

Uh oh!

github-actions bot commented Mar 18, 2026

Uh oh!

github-actions bot commented Mar 20, 2026

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 27, 2026

Uh oh!

github-actions bot commented Mar 29, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

✅ ExtensionParser — `Environment.ExpandEnvironmentVariables` (#3144)

1. Stale comment in `CommandTypeGenerator.cs` (line 222)

2. Dynamo `RequiresMainThread` default change needs confirmation (line 240)

tay0thman commented Mar 16, 2026 •

edited

Loading

Review of latest commits (up to `f21541d`)

🆕 New commit: `f21541d` — "Pass revitYear to ParseComponents function"