Transactions and ViewProviders

[DeepSOIC]

Topic of this thread has changed, so I renamed the thread.
The old title was "How to add python modules to App/Gui?" --DeepSOIC


Hi!
When developing smart visibility in sketch edit, I decided to re-use TempoVis python module I made for AttachmentEditor.

TempoVis is a simple Python module that remembers visibility states of objects that were shown/hidden via its interface, and restores the original visibilities when the TV is deleted (or .restore() method is called explicitly). For Sketcher, I extended it to be able to save/restore camera as well.

It kind of feels funny to make Sketcher dependent on AttachmentEditor, although there is nothing fundamentally wrong with it, since sketcher depends on Part and attachment editor is in Part anyway.

But then came @sliptonic and said he wants to add something like in Path. Path also depends on Part, but you know.. it's a matter of time when someone would want this functionality from a module that does not depend on Part.

So, the module probably needs to be moved to more appropriate place. Somewhere in Gui, in proximity to GuiDocument... There are no Python modules in FreeCAD core I'm aware of, and I don't know how to add one, and where should it be written during build/install.

An alternative option may be to reimplement the module in C++, and make python binding. I'm not very wanting to do this, as Python gives a lot of convenience (e.g. I can easily test of object has Visibility property, and skip it if it doesn't). Another downside is that the module will probably swell by 3-5 times in size. The up side it will be easier to use from within C++ (but I had almost no trouble using Python TempoVis from Sketcher).

Opinions?

[ickby]

As I still have the hope that in the near future GUI changes become part of the transaction system (undo/redo) I would say that the "golden solution" to the temporary visibility problem must be incorporated into the transaction framework, as this is something that should not be recorded and ideally temporary changes would be reset when a transaction/command is commited. So in this regard I would keep your current solution out of the core and maybe make a own module (not workbench) that provides that functionality. This way everyone can use it without the need to load up Part workbench.

But well, I'm totally aware that most likely the Gui transactions are not happening in the next oh so many years and your solution becomes the defacto standart which is hard to change later, so it could go to core anyway. But the "extra module" is IMHO not worse than having it in core, except for visibility to developers maybe.

Would definitely be nice to have something that makes this kind of behaviour simpler!

[DeepSOIC]

maybe make a own module (not workbench) that provides that functionality. This way everyone can use it without the need to load up Part workbench.

Hmm, nice suggestion. Made me thinking . I like the idea!

Extending it a bit, it sounds like "merge part-o-magic" . I actually first wanted to add sketch smart visibility to part-o-magic, but then decided to actually do it inside sketcher, so that everyone can enjoy it . At least, it is something not very related to containers, so it won't be broken by hopefully oncoming container-related changes.

That means I should make the code prepared that the module may not be there, so that editing sketches should still work without polluting report view with errors. Shouldn't be a problem.

[wmayer]

Over the weekend I have worked on extending the undo/redo framework for view providers. Here my branch: https://github.com/wwmayer/FreeCAD/commits/undoredo

When running the unit tests then all tests pass as long as the DAG view is switched off. If it's on then an assert is raised in Gui::DAG::findRecord (line 81) but I don't know why.

Anyway, this branch would fix a list of bugs: issue #0001118, issue #0000819, issue #0001990 and issue #0002054

[ickby]

Over the weekend I have worked on extending the undo/redo framework for view providers. Here my branch: https://github.com/wwmayer/FreeCAD/commits/undoredo

AWESOME

[DeepSOIC]

Interesting.
Does it interfere with my visibility automation in sketcher? Does it mean I need to reimplement it to use new transactions (as of now, I don't see how it's possible)?

Pulling the branch...

[wmayer]

Let me explain some details:
In order to support transactions on document objects and view providers I have added a new class called "TransactionalObject" which DocumentObject and ViewProvider inherit from. The interface for Transaction and TransactionObject has been changed to work with TransactionalObject to also support view providers.

The class TransactionalObject adds two virtual functions isAttachedToDocument and detachFromDocument which is needed to know when an object must be destroyed (e.g. create a box -> undo -> create a cylinder -> destroy the box)

Some methods of TransactionObject are now virtual that are re-implemented in TransactionDocumentObject and TransactionViewProvider. Since the creation of the concrete transaction object still happens in App I have also implemented a factory class TransactionFactory. Instead of having a factory I could have added a virtual function to TransactionalObject and re-implement it in DocumentObject and ViewProviderDocumentObject but I decided against it because otherwise someone could again re-implement this method in further sub-classes and do something unexpected there.

Note, the class Transaction is used by App::Document and this internally holds the transaction objects for document objects and view providers. There is no 2nd transaction object used in the Gui::Document because otherwise I had to implement the same logic for undo/redo in the GUI document and I wanted to avoid this.

In the past Transaction used a std::map for fast access. However, a std::map doesn't keep the order of added objects which is now very important otherwise it can happen that when restoring the document object it creates a new view provider instead of using the saved one. So, the map has been replaced by a list of pairs. Of course, the search is now O(n) instead of O(log(n)) but we don't have documents with thousands of objects. If this becomes a problem I think boost's multiindex class might be an option.

In App::Document two new signals are added which the Gui::Document connects to. This way the Gui::Document will be notified to remove a view provider from the internal map and store it in the transaction or directly destroy the view provider if the passed transaction is null.

The most important changes in Gui::Document are that Document::slotNewObject no longer always creates a new view provider but only when needed and accordingly Document::slotDeletedObject no longer must delete the view provider.

As an example the toggle visibility command has been changed to support undo/redo.

[wmayer]

Does it interfere with my visibility automation in sketcher?

I don't think so. The visibility automation works this way:

save visibility > hide objects depending on sketch > change sketch & leave edit mode > restore visibility

However, the undo/redo system behaves like a stack and your scenario cannot be achieved with the transaction concept of the undo/redo system. If you tried you had to:
open transaction > hide objects depending on sketch > commit transaction > open transaction > change sketch & leave edit mode > commit transaction.

Now if you wanted to restore the visibility you have to undo the transactions twice but then you also lose the changes you made on the sketch. To restore the visibility the undo/redo system needed to be able to keep the last transaction and only undo the second last transaction but in general this is unsafe as you don't know what has changed during the last transaction. For example it could have removed objects from the document.

[wmayer]

When running the unit tests then all tests pass as long as the DAG view is switched off. If it's on then an assert is raised in Gui::DAG::findRecord (line 81) but I don't know why.

From the Document unit tests I could nail it down to the testGroup test that leads to the crash. This is a reduced script showing the behaviour:

Code: Select all

Doc = FreeCAD.newDocument("UndoTest")

# Add an object to the group
L2 = Doc.addObject("App::FeatureTest","Label_2")
G1 = Doc.addObject("App::DocumentObjectGroup","Group")
G1.addObject(L2)

# Remove first object and then the group in one transaction
Doc.openTransaction("Remove")
Doc.removeObject("Label_2")
Doc.removeObject("Group")
Doc.commitTransaction()
Doc.undo() # <<=== crash

[ickby]

Interesting read. One thougth regarding temporary visibility: When a sketch is opened I think a transaction is started. Now all the visibility changes like hiding unrelevant stuff is commited to this transaction. Later they are reversed, the objects are made visible again, which leads to more entries in the transaction. And than the transaction is commited. This means the transaction stores lots of unneccesarry changes, doen't it? As it is a stack I think there is no mechanism to filter out multiple changes to the same property.

But on the other hand same could happen with normal App part proeprties, for example if the user changes the pad high multiple times before "apply". This already works flawless.