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

Keep the revit element with Node when Disable Transaction #2761

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ZiyunShang
Copy link
Collaborator

Purpose

After add Transaction control, an issue was found that element binding was broken when disable and enable transaction. So we need to keep the Revit elementbinder with D4R nodes.
According to design, when a node runs a Revit element, then disable transaction, the node should not turn yellow and display warning because it's not user error.

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

@ShengxiZhang @wangyangshi @saintentropy

FYIs

@QilongTang @mjkkirschner @Amoursol

@@ -100,8 +100,13 @@ public int UnRegisterAssociation(T elementID, Object wrapper)
//ID already existed, check we're not over adding
if (existingWrappers.Contains(wrapper))
{
int index = existingWrappers.FindIndex((x) => object.ReferenceEquals(x, wrapper));
existingWrappers.RemoveAt(index);
var removeList = existingWrappers.FindAll((x) => object.ReferenceEquals(x, wrapper));
Copy link
Member

Choose a reason for hiding this comment

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

what case triggered this change? Could index ever be -1? (not found?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I debug disable/enable transaction, I found that existingWrappers contains multiple wrapper, but no one can be referenceEqueals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This may be debugging code, I will solve this, but I think it would be better to add a judgment protection to the index.

Copy link
Collaborator

@ShengxiZhang ShengxiZhang Nov 8, 2021

Choose a reason for hiding this comment

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

Thank you @ZiyunShang for the update.
I still have concern about this implementation. A lot of nodes need to check the state of DisableTransaction, which requires Dynamo developers to keep in mind that the node may run in DisableTransaction mode, and need to deal with the behavior. And it also requires Dynamo developers to be familiar with the API internal implementation, know if the API calls regen or starts Transaction internally (API can call regen itself, and UI API may start and commit transactions)
Moreover, even doing all above, this only works for the built-in nodes. It doesn't work for package nodes, python nodes, custom nodes, which are also in the AC list.
@saintentropy @QilongTang any ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants