search

OndrejKunc / SkiaScene

Simple API to transform SkiaSharp objects using functions like zoom and rotate or using gestures like pan and pinch.
MIT License
67 stars 18 forks source link

Add element to idToTouchDictionary only if not exists previously #11

Closed rfsouto closed 4 years ago

rfsouto commented 5 years ago

Resolve issue #2 and #1

OndrejKunc commented 5 years ago

Hi,

Thanks for creating the merge request. I think I understand your intention to fix the error in the place where the exception is thrown, but as I described in the #1 and #2 I think it doesn't solve the issue.

The main problem is that in some situations, usually when you have your layout nested inside other scrolling layout, you don't receive the release event.

To be more concrete about the merge request: idToTouchDictionaryrepresents set of all fingers which are pressed right now and you need to remove the finger from the dictionary when the finger is released. But since we don't receive the release event, we can't remove the element from the dictionary. Since each key inside the dictionary is just address of the pointer in the unmanaged memory some random time later the system can reuse the same address for the next finger and that is when exception is thrown.

If we decide to just check if the key already exists it will not fail immediately but can introduce some even more confusing issues:

  1. Memory leaks: We don't remove the fingers from the dictionary and the moment when you add same key is pretty random, so the dictionary can grow really large.
  2. Wrong gesture behavior: Some higher level objects likes TouchGestureRecognizer depend on the correct behavior of the gesture tracking in the lower levels. And when you stop sending the Release action, it may not recognize the gesture and the app will have really strange behavior. Also this object keep his own track of the fingers, so I think you can hit the same exception here.

I think the only acceptable solution would be to make sure we always receive the release event, so you wont't hit this exception in the first place. This is discussed in the mentioned issues and I don't know about any universal solution except to make sure no other view will steal the release event. We can also throw some more descriptive exception so it is less confusing for other users.

Please let me know if you agree with me or prove me wrong :)

rfsouto commented 5 years ago

Hi!

Actually i dont have enough proofs to say if you are right or wrong so i need to accept that you are right.

Only a doubt:

  1. Memory leaks: We don't remove the fingers from the dictionary and the moment when you add same key is pretty random, so the dictionary can grow really large.

=> The fix that i do it´s not about remove keys from dictionary, only to prevent incorrect adding of a existing key that makes exception.

I will chek if i can help in a consistent fix.

Regards.

El lun., 15 jul. 2019 a las 21:38, Ondřej Kunc (notifications@github.com) escribió:

Hi,

Thanks for creating the merge request. I think I understand your intention to fix the error in the place where the exception is thrown, but as I described in the #1 https://github.com/OndrejKunc/SkiaScene/issues/1 and #2 https://github.com/OndrejKunc/SkiaScene/issues/2 I think it doesn't solve the issue.

The main problem is that in some situations, usually when you have your layout nested inside other scrolling layout, you don't receive the release event.

To be more concrete about the merge request: idToTouchDictionary represents set of all fingers which are pressed right now and you need to remove the finger from the dictionary when the finger is released. But since we don't receive the release event, we can't remove the element from the dictionary. Since each key inside the dictionary is just address of the pointer in the unmanaged memory some random time later the system can reuse the same address for the next finger and that is when exception is thrown.

If we decide to just check if the key already exists it will not fail immediately but can introduce some even more confusing issues:

  1. Memory leaks: We don't remove the fingers from the dictionary and the moment when you add same key is pretty random, so the dictionary can grow really large.
  2. Wrong gesture behavior: Some higher level objects likes TouchGestureRecognizer depend on the correct behavior of the gesture tracking in the lower levels. And when you stop sending the Release action, it may not recognize the gesture and the app will have really strange behavior. Also this object keep his own track of the fingers, so I think you can hit the same exception here.

I think the only acceptable solution would be to make sure we always receive the release event, so you wont't hit this exception in the first place. This is discussed in the mentioned issues and I don't know about any universal solution except to make sure no other view will steal the release event. We can also throw some more descriptive exception so it is less confusing for other users.

Please let me know if you agree with me or prove me wrong :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/OndrejKunc/SkiaScene/pull/11?email_source=notifications&email_token=AALESAAWATB44VLSEH7Q373P7TG4PA5CNFSM4IDVA3D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ6XZ5Y#issuecomment-511540471, or mute the thread https://github.com/notifications/unsubscribe-auth/AALESABUBOUHLE5QKPQ2IPTP7TG4PANCNFSM4IDVA3DQ .

OndrejKunc commented 5 years ago

Yes you are right that you don't create memory leaks by this fix. If we don't receive the release event, there will be memory leaks in the current solution as well only with the minor detail that the current solution will randomly fail and your will not. But I think it can fail on the higher levels as I stated before. If you are hitting this issue, have you checked if your gestures are working after the fix?

rfsouto commented 5 years ago

I upload the fix cause in my case the gestures work perfectly. I have a control grid with a template column and an image who change his state with you touch the control. Before the fix, the app crashes randomly and after the fix the crash does not happen again and pass QA test with you project attached.

Regards

El mar., 16 jul. 2019 a las 8:21, Ondřej Kunc (notifications@github.com) escribió:

Yes you are right that you don't create memory leaks by this fix. If we don't receive the release event, there will be memory leaks in the current solution as well only with the minor detail that the current solution will randomly fail and your will not. But I think it can fail on the higher levels as I stated before. If you are hitting this issue, have you checked if your gestures are working after the fix?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/OndrejKunc/SkiaScene/pull/11?email_source=notifications&email_token=AALESAA6BS4JS2EI2LNBLSDP7VSE5A5CNFSM4IDVA3D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ72NAQ#issuecomment-511682178, or mute the thread https://github.com/notifications/unsubscribe-auth/AALESADDW4W7O4IGW4DQA3DP7VSE5ANCNFSM4IDVA3DQ .

Cheesebaron commented 4 years ago

Multi-touch is triggering an exception quite a lot, just spam a bit with two fingers and you will easily trigger it. This PR seems to fix this issue.

We are not nesting the SkiaView inside a ScrollView or anything like that and seeing this a lot.

IainS1986 commented 3 years ago

Is there a release of SkiaScene (or more specifically SkiaScene.TouchManipulation) with this in?

I'm on SkiaScene.TouchManipulation 2.2.0 which appears to just have SkiaScene 1.0.0 internally and TouchTracking 1.1.0