-
Notifications
You must be signed in to change notification settings - Fork 192
d4r for design automation #3152
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: master
Are you sure you want to change the base?
Conversation
OnRequestsCrashPrompt function
Update Greg to 3.0.1
Update Greg to 3.0.1
* update GregRevitAuth to 3.0.8935.26399
* update GregRevitAuth to 3.0.8935.26399
…oRevit into RC3.2.0_Revit2025
* add OOTB templates * update Dynamo Core build to 3.2.1.5366
* add wrapper over Revit RoofBase class to D4R Roof class * cast value data type to parameter value data type in order to avoid blind pointer cast in revit later on * cast value data type to parameter value data type, get parameter value data type from Storage type where value is null
src/Config/CS_SDK.props
Outdated
@@ -1,11 +1,10 @@ | |||
<Project> | |||
<Import Project="$(SolutionDir)Config/user_local.props" /> | |||
<PropertyGroup> | |||
<Platforms>NET70;NET80</Platforms> |
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.
- Do you think platform is appropriate for dotnet versions/ target contexts? I realize you probably don't want to mess with this pattern. I guess my question is - should we be using configuration instead throughout our repos.
- Have you considered what CI/CD might actually invoke this platform build?
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 opted to use Platform because it is integrated with visual studio's UI. It makes it easy to switch.
We can add any string to the platforms list. (we can just have "DA").
The platform values should mostly be visible in the .props file (in the .cs we have pre processor directives). For csproj we can add a new property (UI vs Core or somethign)
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.
CICD can invoke a build either with platform values set, or with other underlying properties (UI vs Core)
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.
part 1 of my review - mostly questions. Will continue pt2 when I can.
src/Config/CS_SDK.props
Outdated
<PlatformTarget >x64</PlatformTarget> | ||
<TargetFramework Condition="'$(Platform)' == 'NET80'">net8.0-windows</TargetFramework> | ||
<TargetFramework Condition="'$(Platform)' == 'NET70'">net7.0-windows</TargetFramework> |
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.
any idea why we even have this net7 target?
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.
probably left over since before net8. Probably removable
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.
Confirm, removable.
src/DADynamoApp/DADynamoApp.csproj
Outdated
</ImportGroup> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>net8.0-windows</TargetFramework> |
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.
isn't the target framework set by the CS_SDK props?
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.
yes, visual studio keeps putting it in here...
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<Compile Remove="obj\**" /> |
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 are these needed?
</Reference> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<PackageReference Include="Autodesk.Forge.DesignAutomation.Revit" Version="2025.0.1" /> |
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.
can this version property use the RevitAPI version - or can we add a TODO to do that when DA supports 2026?
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.
seems like a 2026 version of this package was pushed two weeks ago. Not sure if the versions exactly match.
src/DADynamoApp/DAEntrypoint.cs
Outdated
cmdArgs.DisableAnalytics = true; | ||
cmdArgs.ServiceMode = true; | ||
cmdArgs.AnalyticsInfo = new() { HostName = "Revit_DA" }; | ||
cmdArgs.ImportedPaths = [Path.Combine(curFolder, "nodes", "RevitNodes.dll"), Path.Combine(curFolder, "nodes", "DSRevitNodesUI.dll")]; |
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.
okay, you're going to need to give me some more info as to why you think this approach is better than what Dynamo/DynamoRevit does normally to resolve dependencies?
Or is the crux, that you want to avoid subclassing DynamoModel for some reason?
cmdArgs.ServiceMode = true; | ||
cmdArgs.AnalyticsInfo = new() { HostName = "Revit_DA" }; | ||
cmdArgs.ImportedPaths = [Path.Combine(curFolder, "nodes", "RevitNodes.dll"), Path.Combine(curFolder, "nodes", "DSRevitNodesUI.dll")]; | ||
// need this for cloud, does not work on local |
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 does not work in local?
src/DADynamoApp/DAEntrypoint.cs
Outdated
cmdArgs.CommonDataFolder = Path.Combine(dynTempDir, "Dynamo"); | ||
|
||
// Startup a new project, maybe an option we can have ? | ||
//app.NewProjectDocument(Autodesk.Revit.DB.UnitSystem.Metric); |
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.
If I were a user executing against dynamo as a service - I would expect this information to somehow have been supplied via a web UI or via part of the graph authoring experience ahead of time.
For example:
- I authored the graph in a metric project, so that should be taken into account when run in this context against another project.
- I set this graph to run against a particular project already so we should just open that project.
Do you have a user use case in mind where a graph should be run on a brand new project?
src/DADynamoApp/DAEntrypoint.cs
Outdated
|
||
LoadMessage = model != null ? "loaded" : "no loaded"; | ||
Console.WriteLine($"Checking python in {curFolder}"); | ||
CheckIfPythonExists(curFolder); |
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.
can you explain why you are doing this as opposed to letting Dynamo load python the normal way?
|
||
File.WriteAllText(Path.Combine(WorkItemFolder, "result.json"), result); | ||
|
||
model.RunCompleted += Model_RunCompleted; |
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.
isn't this too late to attach this event? Hasn't the graph already been run via HandleRoute
?
Minimal changes to get D4R working on Design automation
This PR is based on RC3.3.0_Revit2025 because the latest Revit on DA seems to be 2025
I opted to use "if def"s in order to minimize the changes and avoid breaking changes. As a follow up task we should do a proper separation of Core and UI functionality in all the projects (this will most likely mean breaking changes).
I will create a couple of jira tasks with proposed changes to pursue this further.