-
Notifications
You must be signed in to change notification settings - Fork 6
Avoid loading all assemblies by using a pre-compiled menu data file #531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
|
@BHoMBot check installer |
|
@adecler to confirm, the following actions are now queued:
|
|
@BHoMBot check versioning |
|
@adecler to confirm, the following actions are now queued:
|
|
@adecler to confirm, the following actions are now queued:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through the code changes, happy with the overall setup, just a few comments concerning particular decisions
| public UnitTestCaller() : base() | ||
| { | ||
| SetPossibleItems(Engine.UI.Query.EngineItems()); | ||
| IEnumerable<SearchItem> items = Initialisation.SearchItems.Where(x => x.Text.Contains('(')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know the exact details of the UI, but should we not have a UnitTestCaller type filter here rather than checking whether text contains a parenthesis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, all methods are candidate for unit testing as far as I understand. I could filter by looking at all callers that use a method but this is much simple and efficient.
| /**** Private Fields ****/ | ||
| /*************************************/ | ||
|
|
||
| Dictionary<string, List<string>> m_AssemblyNamePerType = new Dictionary<string, List<string>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider changing the type here to an array for performance purposes, assuming these are closed sets that do not change dynamically (and knowing there might be thousands of them)
| Dictionary<string, List<string>> m_AssemblyNamePerType = new Dictionary<string, List<string>>(); | |
| Dictionary<string, string[]> m_AssemblyNamePerType = new Dictionary<string, string[]>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've never used arrays in the BHoM, always lists
BHoM_UI/Global/Initialisation.cs
Outdated
| using System.Threading.Tasks; | ||
| using System.Windows; | ||
| using System.Windows.Controls; | ||
| using System.Windows.Forms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that must have been added when I was exploring different options. I've removed the unnecessary using in this commit: 15aac9c
|
|
||
| public static List<CodeElementRecord> CodeElements { get; set; } = new List<CodeElementRecord>(); | ||
|
|
||
| public static AssemblyResolver AssemblyResolver { get; set; } = new AssemblyResolver(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this property? It feels a bit weird that we have a duplicate property in Engine and UI, which (theoretically) could mismatch. I would rather remove it and default all calls to the engine one, assuming it does not serve any other purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same object. The UI is just passing it to the Engine so it can be access from there.
|
|
||
| // Create the assembly resolver and link it the the BHoM engine | ||
| AssemblyResolver = new AssemblyResolver(assemblyNamesPerType); | ||
| BH.Engine.Base.Compute.SetAssemblyResolver(AssemblyResolver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if it would not be worth making this call a bit more explicit, now it is a bit hidden in a method of lower order (LoadCodeElements being called as one of the 4 methods in a sequence). It is a bit like 'now we populate the UI with items to select, and by the way we are changing the way in which assemblies are being loaded - and only if you read the code carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I have create a comit with that change: 0379593
| { | ||
| List<string> loadedAssemblies = new List<string>(); | ||
|
|
||
| Regex regex = new Regex(@"oM$|_Engine$|_Adapter$"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big fan of this one, it introduces misalignment with BHoM_Engine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how it is misaligned with the BHoM_Engine. The is the exact same filter as used by BH.Engine.Base.Compute.LoadAllAssemblies(): https://github.com/BHoM/BHoM_Engine/blob/develop/BHoM_Engine/Compute/LoadAllAssemblies.cs
| foreach (string file in Directory.GetFiles(BH.Engine.Base.Query.BHoMFolder(), "*.dll", SearchOption.TopDirectoryOnly)) | ||
| { | ||
| string name = Path.GetFileNameWithoutExtension(file); | ||
| if (regex.IsMatch(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following my previous comment, I would suggest using the existing methods to centralise the core classification of assemblies
| if (regex.IsMatch(name)) | |
| if (name.IsOmAssembly() || name.IsEngineAssembly() || name.IsAdapterAssembly()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be convinced of that. As mentioned above, my goal at the moment though is to stay aligned with the previous code from LoadAllAssemblies()
| { | ||
| return Engine.Base.Query.BHoMTypeList() | ||
| .Concat(Engine.Base.Query.BHoMInterfaceTypeList()) | ||
| return Engine.Base.Query.AllTypeList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you make this change here? Trying to follow but can't come up with a reason off the top of my head
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limiting the types registered exclusively to the BHoM objects causes multiple failure on serialisation.
We have separate items for specifically creation BHoM objects via their set of properties or Create methods. So the Types registered here are purely for deserialisation and creating Type objects in the UI.

NOTE: Used by
BHoM/Excel_UI#423
NOTE: Depends on
BHoM/BHoM#1698
BHoM/BHoM_Engine#3561
BHoM/BHoMAnalytics_Toolkit#95
https://github.com/BuroHappoldEngineering/BuroHappold_BHoMAnalytics_Toolkit/pull/82
https://github.com/BuroHappoldEngineering/BuroHappold_Installer/pull/462
BHoM/Versioning_Toolkit#317
Issues addressed by this PR
Closes #521
This PR eliminates the need to load all the assemblies when starting the BHoM UI. Instead, here's how it now works:
Solving the problem at the level of the serialiser means we are taking care of the following problems at the same time:
The file that contains all the information needed to populate the menus is located here:
C:\ProgramData\BHoM\Resources\AssemblyContent.tsvTest files
In both Excel, Rhino 7, and Rhino 8: