Skip to content
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

Update to Dynamo Core 2.8 #2608

Merged
merged 2 commits into from
Aug 17, 2020
Merged

Conversation

mmisol
Copy link
Contributor

@mmisol mmisol commented Aug 7, 2020

Purpose

Includes:

  • Dynamo NuGet update
  • Preloading the Dynamo CPython3 engine
  • Revit customizations for Dynamo CPython3 engine

Known caveats:

  • No tests for new Python engine
  • Dynamo Core is no longer CLS-compliant creating hundreds of warnings
  • Dynamo NuGets for 2.8 are still in beta

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • Snapshot of UI changes, if any.

Reviewers

FYIs

@mjkkirschner @QilongTang

mmisol added 2 commits August 7, 2020 17:36
Includes:
- Dynamo NuGet update
- Preloading the Dynamo CPython3 engine
- Revit customizations for Dynamo CPython3 engine

Known caveats:
- No tests for new Python engine
- Dynamo Core is no longer CLS-compliant creating hundreds of warnings
- Dynamo NuGets for 2.8 are still in beta
@mjkkirschner
Copy link
Member

Any idea when / how we lost CLS compliance? Not that this is particularly important IMO... but we may want to silence those.

@mmisol
Copy link
Contributor Author

mmisol commented Aug 10, 2020

Yes, it was here DynamoDS/Dynamo#10668. I was told by @QilongTang this was resolved as a way to silence SQ warnings. I was trying to see if we had other ways of dealing with this, maybe silencing the warnings in SQ itself or something like that.

@QilongTang
Copy link
Contributor

@mjkkirschner Was discussed in slack when first time we tackle SQ warnings, according to SQ we are not really following CLS compliant rules so less meaningful to keep that setting. Let me know.

@mjkkirschner
Copy link
Member

oh, I see, perhaps it would be appropriate to add a pragma to DynamoRevit disabling the warnings if we have no intention of being CLS compliant - I know in some cases we can't - because we can't change API names at this time.

<Private>False</Private>
</Reference>
<Reference Include="Python.Runtime">
<HintPath>$(PACKAGESPATH)\pythonnet\lib\net40\Python.Runtime.dll</HintPath>
Copy link
Member

Choose a reason for hiding this comment

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

what was this actually needed for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A reference to PyScope is required when adding the UnwrapElement function.

@ZiyunShang
Copy link
Collaborator

Hi @mjkkirschner @QilongTang , @AndyDu1985 is on vacation now, and I should confirm whether we want to upgrade to DynamoCore 2.8.0 now and will be released next version.

@QilongTang
Copy link
Contributor

@ZiyunShang Is there a way we can get Dynamo 2.8 on Revit master branch? I think this PR is the suggested work

CPythonEvaluator.EvaluationEnd += (a, b, c, d) => ElementBinder.IsEnabled = true;

// register UnwrapElement method in cpython
CPythonEvaluator.EvaluationBegin += (a, scope, c, d) =>
Copy link
Member

Choose a reason for hiding this comment

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

should we be unsubscribing these when python nodes are removed from the workspace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't get that @mjkkirschner . These events are static, not really associated to a Python node in particular.

Copy link
Member

Choose a reason for hiding this comment

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

whoops! my mistake.

@QilongTang
Copy link
Contributor

@AndyDu1985 @ZiyunShang Is it fine to merge now or you would rather wait for the final RC2.8.0 to be out by end of month to do a single sweep?

@AndyDu1985
Copy link
Collaborator

@AndyDu1985 @ZiyunShang Is it fine to merge now or you would rather wait for the final RC2.8.0 to be out by end of month to do a single sweep?

It's OK have this version in DynamoRevit.

@AndyDu1985 AndyDu1985 merged commit 65e2e52 into DynamoDS:master Aug 17, 2020
ZiyunShang added a commit to ZiyunShang/DynamoRevit that referenced this pull request Sep 3, 2020
ZiyunShang added a commit that referenced this pull request Sep 3, 2020
* Revert "Update to Dynamo Core 2.8 (#2608)"

This reverts commit 65e2e52.

* Update for RC2.6.1_RevitMaster
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.

5 participants