Skip to content

Conversation

@romangolev
Copy link
Contributor

@romangolev romangolev commented Apr 14, 2025

Rewrite Loader into C#

Description

Implementing an AssemblyBuilder, UIBuilder and SessionManager refactored in C# as a substitute for the original Loader which is written in Python.
This feature aims to increase the load speeds on startup and reload events.


Checklist

Before submitting your pull request, ensure the following requirements are met:

  • Code follows the PEP 8 style guide. -- Not applicable
  • Code has been formatted with Black using the command: -- Not applicable
    pipenv run black {source_file_or_directory}
  • Changes are tested and verified to work as expected. - No, just a draft for now

Related Issues

If applicable, link the issues resolved by this pull request:

  • N/A

Additional Notes

THIS IS A DRAFT
Entry point is at PyRevitLoaderApplication which now have 2 modules for loading pyRevit sessions:

  • legacy: ExecuteStartUpPython
  • refactored: ExecuteStartUpCsharp

Adds new project pyRevitAssemblyBuilder which is set as a dependency project for pyRevitLoader and pyRevitRunner.
Whenever one of them built, dependency is built and loaded automatically.

pyRevitAssemblyBuilder is using DI (Dependency Injection) and have a set of services being instantiated on Revit startup as singletons.

On the moment of opening this draft PR, the module is able to build dummy dlls and simplified UI for them (without images and tooltips). Yet does not support yaml bundle files, does not build lib files.

Reviewers

@jmcouffin @sanzoghenzo @dosymep what do you think? Is it a way to go?


Thank you for contributing to pyRevit! 🎉

devloai[bot]

This comment was marked as outdated.

@sanzoghenzo
Copy link
Contributor

Thanks @romangolev for the huge job!

If it was me, I would already got rid of the legacy/ExecuteStartUpPython method and the IronPython dependency, so that we can put the dlls into the bin/netcore and bin/netfx regardless of the selected python engines.

for the bundle files, I had an idea a while ago to simplify things and switch from folder-based extensions to manifest-based extensions, I'll open a feature request to see if I'm the only one or if this could be a valuable feature that could already be implemented in this PR

@dosymep
Copy link
Member

dosymep commented Apr 15, 2025

manifest-based extensions

I think if we create good architecture, we can support two way to create tabs :)

@dosymep
Copy link
Member

dosymep commented Apr 15, 2025

@romangolev Good Job!

I have some doubts about the DI container from Microsoft, we can get into a situation with dll hell and I would like to see a more OOP architecture of this. It would be cool to separate the structure parser from the main libs into a separate one, where there would be no dependence on revit, python and other things, only pure code (service) for parsing the structure.

To implement these things that I listed, you need a lot of effort, and make many important decisions, so I think, for a start, to get rid of the python code, your implementation is perfect.

@jmcouffin
Copy link
Contributor

manifest-based extensions

I think if we create good architecture, we can support two way to create tabs :)

Agreed; but if we start supporting extra worklflows, I will need pyRevit to provide me a paid job ;o
Joke aside, I like the idea of manifest-based extensions:

  • Simplifies greatly the extensions part of the code base
  • Allow for a specific UX to create those
Side projects I work on that could leverage a manifest based extensions and streamline extension creation

WIP: https://builder.pyrevitlabs.io/

@romangolev
Copy link
Contributor Author

@sanzoghenzo it's just the start! The legacy loader has quite a lot of code on and because of that, I believe that gradual approach is preferable. I would say that after implementing this feature we may end up with some unpredictable bugs, so for users to be able to make a step back and activate the old way of loading things might be a walk-around in order not to lose main functionality.

@romangolev
Copy link
Contributor Author

@romangolev Good Job!

I have some doubts about the DI container from Microsoft, we can get into a situation with dll hell and I would like to see a more OOP architecture of this. It would be cool to separate the structure parser from the main libs into a separate one, where there would be no dependence on revit, python and other things, only pure code (service) for parsing the structure.

To implement these things that I listed, you need a lot of effort, and make many important decisions, so I think, for a start, to get rid of the python code, your implementation is perfect.

@dosymep thanks man🙏
I had a doubt about the packages as well. Should we degrade this and implement execution without DI? I'll dig it to see if it's a big deal, but yeah, in case somebody else loads that same dlls, the result might be unpredictable.

Also, the Parser itself doesn't have a lot now, so it wouldn't be a problem to make a separate csproject from it.

@jmcouffin
Copy link
Contributor

@romangolev how is your progress, I just saw the commits but cannot really test, I need to keep my dev environment clean for a presentation next week.
Let me know if you need me/us to test ride the thing.

@romangolev
Copy link
Contributor Author

@jmcouffin these are the commits regarding to the comments of @dosymep
I managed to got rid of DI and moved the extension parser to a separate file.
That's it for the moment.
Next move is going to be implementing functionality gradually to make it work similar like a python loader 😎
So yeah, probably a little early for a full fledged testing

@romangolev
Copy link
Contributor Author

romangolev commented Nov 24, 2025

@jmcouffin @dosymep @sanzoghenzo It is not the cleanest code I've written, but at this point I expect most of the features to work correctly with the new loader. Please take a look, drop me your concerns please, I am willing to make some adjustments anyways.

@romangolev romangolev marked this pull request as ready for review November 24, 2025 12:07
Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the huuuge work!

I still have to review the longer files and the Parser Tester, but since I don't know if/when I can finish them, I figured I can start bugging you with my pedantic comments right away 🤣

Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second round 😉

@romangolev
Copy link
Contributor Author

Parser Tester

This solution contains Unit tests that confirm the correct behavior of the Parser module.
Basically tests that everything is parsed correctly, the bundle files and the data inside the scripts

sanzoghenzo
sanzoghenzo previously approved these changes Nov 30, 2025
Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! I just added two comments about using regexes instead of multiple Contains/EndsWith/Replace, but they're not that important 😉

Copy link
Member

@dosymep dosymep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests I didn't check :)

@@ -0,0 +1,12 @@
title:
fr_fr: TEST TITLE 1 FR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write in the description of the test buttons what is being tested, otherwise it is clear that there is only one icon, and only for the dark theme, and it is not clear whether this is intended or they simply forgot to add an icon for the light theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been used in testing suite. I am also using the approach where I create files on-the-go for testing purposes and then delete them. Do you think this would be a more desired approach?

Comment on lines +43 to +44
public ParsedBundle Bundle;
public DateTime LastModified;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fields muste be private or use public properties

switch (key)
{
case "clean":
parsed.Engine.Clean = rawValue.ToLowerInvariant() == "true";
Copy link
Member

@dosymep dosymep Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rrawValue.Equals("true", StringComparison.InvariantCultureIgnoreCase)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have used the YamlDotNet library to parse YML files

This library is used for parsing YML in Python.

Comment on lines 77 to 86
return fileName.Contains(".dark.") || // icon.dark.png
fileName.Contains("_dark.") || // icon_dark.png
fileName.Contains("-dark.") || // icon-dark.png
fileName.Contains("_dark_") || // icon_dark_theme.png
fileName.Contains("-dark-") || // icon-dark-theme.png
fileNameWithoutExtension.EndsWith("_dark") || // icon_dark.png
fileNameWithoutExtension.EndsWith("-dark") || // icon-dark.png
fileNameWithoutExtension.EndsWith(".dark") || // icon.dark.png
(fileNameWithoutExtension.Contains("_dark") && !fileNameWithoutExtension.StartsWith("dark")) || // icon_dark_theme but not dark_icon
(fileNameWithoutExtension.Contains("-dark") && !fileNameWithoutExtension.StartsWith("dark")); // icon-dark-theme but not dark-icon
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use one template icon.dark.png

Why other options?

var fileName = FileName.ToLowerInvariant();

// Look for size patterns like "_16", "_32", "_64", "16.png", etc.
var sizePatterns = new[] { "16", "32", "64", "128", "256" };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In methods that are called many times, it is not recommended to create lists and arrays for one use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this array either static or a class field, the second option is preferable

@romangolev
Copy link
Contributor Author

tests I didn't check :)

I will recheck them once again myself, no worries. They are more for the developing and maintainting. I've implemented test driven approach while working on my main task.

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.

8 participants