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

bug(revit): CNX-8730 deletes existing objects in a subtransaction during receive #3209

Conversation

clairekuang
Copy link
Member

Description & motivation

When receiving in Revit in update mode, always deletes existing objects in a subtransaction. This fixes a bug where in certain conditions, attempts to delete existing objects are made without any active transaction.

This bug turned out to be completely unrelated to Sketchup direct shape mappings, but direct shapes are one of few objects that are properly "updated" (an existing direct shape object's shape is modified in update mode, vs the existing object being deleted and a new ds being created).

The following reproduction steps create the "failed update" behavior:

  1. receive a direct shape
  2. receive a new version of the same direct shape (same app id) on update mode -> success
  3. receive a new version of a different direct shape (different app id) on update mode -> failure, existing directshape isn't deleted.

While my changes appear to fix the failed updating behavior described above, I'd like @connorivy to take a look at the proposed changes because it's still unclear to me why no active transaction existed (my guess is that passing the transaction manager to the converter, editing the direct shape in the converter closed the transaction, and therefore no open transaction was available when the connector attempts to delete the existing object ). There may be a more robust way to handle transactions due to the differences in direct shape editing in the converter compared to most other conversion routines.

To-do before merge:

@clairekuang clairekuang changed the title dbug(revit): CNX-8730 deletes existing objects in a subtransaction during receive bug(revit): CNX-8730 deletes existing objects in a subtransaction during receive Mar 4, 2024
@clairekuang clairekuang requested a review from connorivy March 4, 2024 16:31
@JR-Morgan JR-Morgan added this to the 2.19 milestone Mar 11, 2024
Copy link
Contributor

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

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

Changes seem legit but I'll defer to @connorivy since my knowledge of the use of TransactionManager is limited.

@@ -333,6 +337,7 @@ void ConvertNestedElements(Base @base, ApplicationObject appObj, bool receiveDir
if (index % 50 == 0)
{
transactionManager.Commit();
transactionManager.Start();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @clairekuang your changes look good. One of the reasons that this issue was happening is because this loop was exiting when index % 50 == 0 (aka it was the 0th object) which was leaving the transaction in a closed state. That was my mistake in originally making this, the transaction should continually be open until we call "Finish" because anyone looking at it would assume that it is open until that point.

It is still good to wrap the object deleting in a subtransaction like you did, but this line is necessary so that this doesn't happen with some other method in the future

@clairekuang clairekuang merged commit f8a6850 into dev Mar 14, 2024
9 checks passed
@clairekuang clairekuang deleted the CNX-8730-DirectShapes-created-from-SketchUp-Mapper-not-updating branch March 14, 2024 09:56
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.

4 participants