forked from DynamoDS/DynamoRevit
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fetching Parameters By Name #1
Open
dimven-adsk
wants to merge
2
commits into
reope/performance-base
Choose a base branch
from
reope/performance-parameters
base: reope/performance-base
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
Mikhinja
approved these changes
Aug 16, 2024
Mikhinja
reviewed
Aug 28, 2024
@@ -8,7 +8,7 @@ | |||
<TestOutputPath Condition=" '$(TestOutputPath)' == '' ">$(OutputPath)</TestOutputPath> | |||
</PropertyGroup> | |||
<PropertyGroup> | |||
<REVITAPI Condition=" !Exists('$(REVITAPI)') ">$(SolutionDir)..\lib\Revit 2025\net8.0</REVITAPI> | |||
<REVITAPI Condition=" !Exists('$(REVITAPI)') ">C:\Program Files\Autodesk\Revit 2025</REVITAPI> |
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.
Also, in the future can you, please, add an entry instead of replacing this one? The current path is used by the internal build pipeline to download Revit API. Just adding another with the current condition !Exist('$(REVITAPI)')
should work just as well.
dimven-adsk
pushed a commit
that referenced
this pull request
Oct 3, 2024
dimven-adsk
added a commit
that referenced
this pull request
Oct 31, 2024
commit 8cc2f05 Author: DimitarVen <[email protected]> Date: Thu Oct 3 16:55:34 2024 +0300 bring improvements from #1 commit e22f851 Author: DimitarVen <[email protected]> Date: Thu Oct 3 14:40:13 2024 +0300 fix for parameter missing commit 9933dc6 Author: DimitarVen <[email protected]> Date: Thu Oct 3 14:28:01 2024 +0300 use internal element def commit 30e8dfc Author: DimitarVen <[email protected]> Date: Thu Oct 3 14:24:03 2024 +0300 store definitions for the duration of the graph execution
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(moved from DynamoDS#3085)
List of Affected Nodes/Modules
Element.GetParameterByName
Element.SetParameterByName
Current Performance
There is a two-fold penalty with the current method. The first one is because every single parameter is fetched for every element and the second is because the entire definition is fetched for each of the parameters in the resulting sequence when all we need is the parameter's name
Proposed Performance
Both of these can be avoided by using the GetParameters(string) method instead. That way fetching the definition is completely avoided and the parameter filtering happens in the native side.
Dynamo Tuneup Comparison
On a model with ~55k elements fetching a single parameter value we drop form 14000ms to 6000ms
Checklist