search

SceneView / sceneview-android

SceneView is a 3D and AR Android Composable and View with Google Filament and ARCore. This is a Sceneform replacement in Kotlin
Apache License 2.0
758 stars 151 forks source link

Re: issue#435, ensure that duplicate calls to destroy arcore are ignored #451

Closed kmayoral closed 3 months ago

kmayoral commented 3 months ago

Since it takes a few seconds to destroy the arcore object, multiple calls to the method can still happen concurrently while the first is still being destroyed but before the object reference has been nullified. This should protect against that race condition.

Antiglobalist commented 3 months ago

@kmayoral Hi! As you can see in my PR I removed destroyARCore() from override fun destroy() https://github.com/SceneView/sceneview-android/pull/438 I wrote a comment that I'm not sure if I need to call this in two places

Now I see that in version 2.1.0 the second call was returned. Perhaps the one who returned it understands the logic better and this call is necessary. But, apparently because of this, the crash returned again

I see your fix. You have added a flag But maybe one of the owners will explain us, why do we need the second call of destroy? image

I see only one case, when ARScene view will not have lifecycle image

I think next (it is not a best solution) solution can prevent double destroy also image

I don't know if it is possible use ArSceneView without Lifecycle My opinion is that the design code of this class has an error We can pass sharedLifecycle: Lifecycle? = null to constructor. And it calls - image Then we can also setup lifecycle, and it calls also - image If i see right this case will call twice Core logic - image

It can produce new error in the future What do you think?

Antiglobalist commented 3 months ago

So, the main questions: 1 - Do we really need to be able to pass two different Lifecycles? 2 - Is it possible to leave one and make it mandatory?

I don't understand why they are needed in construcor. For compose view ?

sharedActivity: ComponentActivity? = null,
    sharedLifecycle: Lifecycle? = null,

Is it possible to change the subscription method to this? Then there will be one lifecycle image

Cuz it looks like only 1 Lifecycle is necessary for ArCore calls

kmayoral commented 3 months ago

Hi @Antiglobalist , thanks for the comments! So it seems that your changes were probably wiped by a conflict resolving merge somewhere along the lines.

I definitely wouldn't be opposed to both changes being included in the code, but I do feel that this PR will make it so any other accidental double calls of destroy() don't cause issues by making this action more idempotent to begin with.

I would like if we could land this PR and then have another PR that reopens the changes you had previously made, what do you think?

Antiglobalist commented 3 months ago

@kmayoral Hi! Sure! I just noticed a common problem :)

Antiglobalist commented 3 months ago

@grassydragon Hello. Please take a look at my research above. Perhaps it makes sense for the future

ThomasGorisse commented 3 months ago

@Antiglobalist The double Lifecycle was there for Flutter but it might not be applicable anymore. So could you please make it mandatory and unique in another PR?

Antiglobalist commented 3 months ago

@ThomasGorisse Sure!