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

Let assembly reloading bypass undo/redo to improve editor stability #2108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrsch
Copy link
Contributor

@adrsch adrsch commented Jan 15, 2024

PR Details

Hack to prevent serious editor bugs on undo/redo for assembly reloads, while leaving the code for it all intact. Introduces poor UI behavior on the Edit History tab, but that's never caused anyone to lose work.

Description

Add a transaction flag for bypassing the undo/redo system, which should not be used except for hacks.

Tested by moving things around and/or editing properties, doing stuff to cause an assembly reload, and then using undo. The things done before the editor reload, successfully undo.

Related Issue

#2107

Motivation and Context

Pain

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@adrsch
Copy link
Contributor Author

adrsch commented Jan 15, 2024

@dotnet-policy-service agree

@Kryptos-FR
Copy link
Member

That's not the way to do it. There is already infrastructure in place to have not-cancellable operations.

With that said, I seem to remember (old story from when I was at SiliconStudio) that there was a reason we still needed that operation to be cancellable. I think it was related to serialization.

@Kryptos-FR
Copy link
Member

In also added a comment in the related issue. At this stage, unless it can be reproduced and confirmed, a hack should not be merged without measuring its consequences.

@adrsch
Copy link
Contributor Author

adrsch commented Jan 15, 2024

That's not the way to do it. There is already infrastructure in place to have not-cancellable operations.

With that said, I seem to remember (old story from when I was at SiliconStudio) that there was a reason we still needed that operation to be cancellable. I think it was related to serialization.

I figured this hack was terrible!

Hopefully the standard reproduction I wrote up on the issue a few minutes ago, using a sample project, can help it get fixed properly, I mostly just figured it would be good to get a hack out there. Since this seems like a thing some people will run into not at all, or all the time, depending on their habits with using undo in the editor.

@adrsch
Copy link
Contributor Author

adrsch commented Jan 15, 2024

I got the true cause figured out of duplicate scripts getting added in #2107 (comment)

Not sure how to fix, it seems like a problem that has got to have been addressed elsewhere in the engine (an item getting processed twice, once as a member of a scene, and second as a member of a prefab)

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.

None yet

2 participants