Using `std::shared_ptr` for `UBGraphicsScene`

[kaamui]

By trying to use shared_ptr, I encountered some particular things that I would like to at least point out so you know it too, or better have your pov on :

• UBGraphicsScene is often passed to Qt functions, so a lot of conversions (from shared_ptr<UBGraphicsScene> to QGraphicsScene* and on the opposite way too of course) are performed. Any concern about that ? I'm a bit concerned wheter we're going to succeed in avoiding Qt from destroying an object that we handle with a smart pointer on our side.
• I automatically every qobject_cast static_cast etc by dynamic_cast, but I think that in a lot of cases, we can guarantee that we're going to have a UBGraphicsScene and so we can use static_cast when possible. Agree ?
• connect functions don't take smart pointers as argument
• I used a shared_ptr even in the worker (so no need to connect to onScenePersisted to delete the copy)
• I also used shared_ptr for blackScene and transparentScene, but I think it may be a point of discussion.

I shared the conversion of the code in the dev-shared-scene. It actually compiles but crashes immediately, because of an early UBGraphicsScene destruction... ^^

[letsfindaway]

By trying to use shared_ptr, I encountered some particular things that I would like to at least point out so you know it too, or better have your pov on :

• UBGraphicsScene is often passed to Qt functions, so a lot of conversions (from shared_ptr<UBGraphicsScene> to QGraphicsScene* are performed. Any concern about that ? I'm a bit concerned wheter we're going to succeed in avoiding Qt from destroying an object that we handle with a smart pointer on our side.

No, not in this direction. I do not see that Qt will ever delete a scene on its own.

• I automatically every qobject_cast static_cast etc by dynamic_cast, but I think that in a lot of cases, we can guarantee that we're going to have a UBGraphicsScene and so we can use static_cast when possible. Agree ?

Do you worry about the performance overhead of a dynamic cast? In principle I agree that a static cast will be sufficient in many cases. But I normally still don't use it and ask myself: If I know by now that it works, can I also guarantee that it always works in the future?

• connect functions don't take smart pointers as argument

As far as I read they can, if you register the shared ptr with the metatype system.

• I used a shared_ptr even in the worker (so no need to connect to onScenePersisted to delete the copy)

Agreed.

• I also used shared_ptr for blackScene and transparentScene, but I think it may be a point of discussion.

They should also use it. I'm even thinking of hiding the plain constructor of the scene and adding a factory method returning a new scene in a shared ptr to avoid that anybody can create a plain scene object.

I shared the conversion of the code in the dev-shared-scene. It actually compiles but crashes immediately, because of an early UBGraphicsScene destruction... ^^

Let's see! What definitely is not a good idea is to convert a plain pointer e.g. retrieved from view->scene() to a shared_ptr. Here we need std::enable_shared_from_this to convert the plain pointer to the shared pointer.

[kaamui]

Let's see! What definitely is not a good idea is to convert a plain pointer e.g. retrieved from view->scene() to a shared_ptr. Here we need std::enable_shared_from_this to convert the plain pointer to the shared pointer.

Yes, I didn't know about that, I just read it here : https://stackoverflow.com/questions/11711034/stdshared-ptr-of-this

[kaamui]

They should also use it. I'm even thinking of hiding the plain constructor of the scene and adding a factory method returning a new scene in a shared ptr to avoid that anybody can create a plain scene object.

I actually don't like this pattern at all. On a general manner, imho design patterns should never transpose in the code in a way that you can actually see the pattern itself. If so, than there is an architectural issue that you did not identified. It's my way of thinking/working.

[kaamui]

Do you worry about the performance overhead of a dynamic cast? In principle I agree that a static cast will be sufficient in many cases. But I normally still don't use it and ask myself: If I know by now that it works, can I also guarantee that it always works in the future?

Not worried, about the performance overhead, I don't think it would be that noticeable, but I basically learned that when you are 100% sure of the object returned by the cast, you should use a static_cast, dynamic_cast for other cases. And that if you're not sure, dynamic_cast... and if you need reinterpret_cast, you probably have an architectural issue ^^

[letsfindaway]

Let's see! What definitely is not a good idea is to convert a plain pointer e.g. retrieved from view->scene() to a shared_ptr. Here we need std::enable_shared_from_this to convert the plain pointer to the shared pointer.

Yes, I didn't know about that, I just read it here : https://stackoverflow.com/questions/11711034/stdshared-ptr-of-this

Just pushed a commit to the branch on my repository introducing shared_from_this. And now it seems to work at first glance :)

[letsfindaway]

Do you worry about the performance overhead of a dynamic cast? In principle I agree that a static cast will be sufficient in many cases. But I normally still don't use it and ask myself: If I know by now that it works, can I also guarantee that it always works in the future?

Not worried, about the performance overhead, I don't think it would be that noticeable, but I basically learned that when you are 100% sure of the object returned by the cast, you should use a static_cast, dynamic_cast for other cases. And that if you're not sure, dynamic_cast... and if you need reinterpret_cast, you probably have an architectural issue ^^

The emphasis is on 100% and I would say: At the places you use the cast, you cannot be 100% sure, because this depends on code in other parts of the program. Is really nobody using another type of scene? You can only answer the question having the view on the complete project, but not from the class or function where you perform the cast. So I would argue: No, you are not 100% sure and you should therefore use a dynamic cast AND check the result.

[letsfindaway]

Should I create a PR to your repository or will you just fetch the commit from mine?

[kaamui]

Should I create a PR to your repository or will you just fetch the commit from mine?

I'll merge my dev-shared-scene to dev, so you can submit a PR on dev directly

[letsfindaway]

And I will have to perform some more tests. It now crashes on exit.

[kaamui]

AND check the result.

yep, that's quite the issue on all the ::scene I converted. I'm not testing the dynamic_cast before creating a shared_ptr of it.

[letsfindaway]

And I will have to perform some more tests. It now crashes on exit.

Currently I have no idea why it crashes. There is no usable stack trace, so the stack seems to be completely corrupted. It happens when the UBPersistenceWorker does its final round, but the actual cause may be earlier.

[kaamui]

Submit your code as-is, it is probably not related at all. I'll see if I can reproduce it or have more info.

[kaamui]

And I will have to perform some more tests. It now crashes on exit.

Currently I have no idea why it crashes. There is no usable stack trace, so the stack seems to be completely corrupted. It happens when the UBPersistenceWorker does its final round, but the actual cause may be earlier.

Is it systematic ?

[kaamui]

I just applied your commit on my branch and cannot reproduce the issue you're experiencing with the UBPersistenceWorker.

But it does systematically crashes at PDF import, on my side. Another issue.

[letsfindaway]

Yes, my crash also is systematic.

What I found out was the following problem when passing around shared pointers between threads: It might be that the last shared pointer goes out of scope in another thread as the one where it was created. This is a problem for Qt. With some tweaking I brought it to the situation where an error message was issued: "QObject::~QObject: Timers cannot be stopped from another thread". And this happened when a UBGraphicsScene was deleted because the last shared pointer went out of scope in the UBPersistenceWorker.

So we have to take care about the owning thread when passing shared pointers. I think this is possible, and I will look into that now.

[letsfindaway]

Just added a single line and pushed a new commit. Now the PDF import is fixed and also my problem is gone.

Can you think of any other place where scenes are processed in another thread? Those would probably also be affected.

[kaamui]

Just added a single line and pushed a new commit. Now the PDF import is fixed and also my problem is gone.

Can you think of any other place where scenes are processed in another thread? Those would probably also be affected.

I can think of createDocumentProxiesStructure, where UBDocumentProxy objects are instantiated in a separate thread, (but I don't think scenes are loaded at this point), also thumbnails that imply a scene loading but it is actually done in the main thread, and finally the PDF Rendering where the PDF's scene might be read from another thread. I didn't look at the code, that's what I can remember, quickly, not sure the scene's actually involved in the other threads that much.

[letsfindaway]

Just added a single line and pushed a new commit. Now the PDF import is fixed and also my problem is gone. Can you think of any other place where scenes are processed in another thread? Those would probably also be affected.

I can think of createDocumentProxiesStructure, where UBDocumentProxy objects are instantiated in a separate thread, (but I don't think scenes are loaded at this point), also thumbnails that imply a scene loading but it is actually done in the main thread, and finally the PDF Rendering where the PDF's scene might be read from another thread. I didn't look at the code, that's what I can remember, quickly, not sure the scene's actually involved in the other threads that much.

This is not a problem as UBDocumentProxy is no longer a QObject. The threading problem only applies to QObjects.

[kaamui]

OK.

I have a double free or corruption, when importing twice the same pdf. Systematic. Zero info from the stack trace. Can you reproduce it ? Do you think it can be related to what you were worried ?

[letsfindaway]

Yes, I can reproduce it.

Please remove this line to correct it:

https://github.com/letsfindaway/OpenBoard/blob/d8e16a5985288295dd7349d3c494f91e762e7fde/src/core/UBSceneCache.cpp#L135

[kaamui]

scene->deleteLater();

Gosh how did you find this ?!

[letsfindaway]

I got a stack trace where the destructor of UBGraphicsScene was called from the event loop. Then I thought about deleteLater, which does exactly this. Then I did a search using the regular expression "scene.*deleteLater" and found that occurrence.

[letsfindaway]

seems to work now. PR was merged

[letsfindaway]

They should also use it. I'm even thinking of hiding the plain constructor of the scene and adding a factory method returning a new scene in a shared ptr to avoid that anybody can create a plain scene object.

I actually don't like this pattern at all. On a general manner, imho design patterns should never transpose in the code in a way that you can actually see the pattern itself. If so, than there is an architectural issue that you did not identified. It's my way of thinking/working.

Here I don't agree and still think the factory method would be useful and justified for the following reasons:

• UBGraphicsScene is designed in a way that it is only usable when it is wrapped with a shared_ptr. We use shared_from_this() four times in the code of this class. And this call fails horribly when the class is not wrapped with a shared_ptr. See https://en.cppreference.com/w/cpp/memory/enable_shared_from_this. From the Notes:

It is permitted to call shared_from_this only on a previously shared object, i.e. on an object managed by std::shared_ptr. Otherwise the behavior is undefined (until C++17)std::bad_weak_ptr is thrown (by the shared_ptr constructor from a default-constructed weak_this) (since C++17).

• Being wrapped in a shared_ptr and the usage of shared_from_this() is not only an implementation detail. It is part of the class' API and shall therefore be enforced by the compiler as far as possible. Things like gsl::not_null, Assert etc. do the same and help improving code quality.
• A class' internal code shall not depend on the way how it is used externally. So if it can enforce to be wrapped in a shared_ptr, it shall do so. This is the same (or a similar) argument as for using const on return values or private on fields and functions. You force the user of the class to behave in a specific way.
• Let the compiler detect errors, not the runtime. That's also the reason why I delete the copy constructor and assignment operator from classes managed by a shared_ptr. Such classes are meant to be passed by pointer, not by copying. Note that all QObject derived classes automatically have that. See https://doc.qt.io/qt-5/qobject.html#no-copy-constructor-or-assignment-operator. The reasons are similar: a QObject is meant to be managed by a parent. A copy is likely not what you expect. Deleting the copy constructor causes the compile to throw an error if you try to pass a QObject by value or something like that.

[letsfindaway]

I would even think about creating a factory method for the UBDocumentProxy. Here you're still able to create two instances representing the same document. This should not be the case.

Instead we could have a factory method which looks for an existing proxy and returns that if it exists. Else it creates a new one.

This can be realized by keeping a QMap<QString,std::weak_ptr<UBDocumentProxy>> somewhere, most probably as static member of UBDocumentProxy. The document path serves as key. When a new UBDocumentProxy is created, then we add it to this map. In the destructor, the UBDocumentProxy removes itself from that map.

Default-constructed proxies are added to the map when the persistence path is set.

I reopen this issue as the discussion has probably not ended ;)

[kaamui]

Here I don't agree and still think the factory method would be useful and justified for the following reasons:

I may have not explained correctly what I mean. I just don't like when the usage of the factory pattern is explicit (I don't want to see a FooFactory class in the code, and it is generally made explicit like this).

I'm OK with the idea to "give the responsibility to a specific class to create documents". It is actually something I was thinking of doing too, something like a ResourcesManager or a Documents Manager (responsible of creating, giving access, handling and deleting documents and maybe other resources).

UBPersistenceManager is already close to be assuming this role for documents. I would be OK with UBDocumentProxy constructors being only callable by a class like UBPersistenceManager, assuming creation of documents.

In correlation, there is an issue (or at least an opposite design direction) with all the deepCopy functions.

[kaamui]

This can be realized by keeping a QMap<QString,std::weak_ptr<UBDocumentProxy>> somewhere, most probably as static member of UBDocumentProxy. The document path serves as key. When a new UBDocumentProxy is created, then we add it to this map. In the destructor, the UBDocumentProxy removes itself from that map.

I would prefer it to be in a dedicated static class or namespace, where it would be clearer/easier to distinguish between UBDocumentProxy global operations (create, get, clone, remove) and internal ones (setPageCount, setLastVisitedSceneIndex)

[letsfindaway]

Another word about factories: while I second your opinion here, I think there are uses of a factory and for OpenBoard 2.0 I was thinking about using an abstract factory for the built-in tools, to create them from the URL, instead of having long if-then-else chains.

In an abstract factory, the factory does not know which classes it creates. It just keeps a map, mapping a class identifier (here the openboardtool: URL) to a create() function. The classes register themselves at the factory using a static initializer.

Currently the UBToolsManager has a similar role, but it has to provide the tool descriptors by its own. They should better be contributed by the tools. And the creation of tools is that big if-then-else in UBBoardController:1444ff. And the scene has methods for each of the tools. Proper use of a factory could harmonize that an make the code much cleaner.