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

fix: prevent null exception when undefined/null Polygon.hitValue #1828

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

JaffaKetchup
Copy link
Member

This works in the polyline layer, but I missed out a line copy when carrying some changes over.
Thanks to @mohammedX6 for bringing this to my attention. Replaces #1826.

josxha
josxha previously approved these changes Feb 14, 2024
Copy link
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry that I haven't seen this in my review.

edit: will do some additional testing, before I approve 😁

@josxha josxha self-requested a review February 14, 2024 13:24
@josxha
Copy link
Contributor

josxha commented Feb 14, 2024

When you zoom in far like showcased in the video and then hover over the polygons the app crashes.

polygon.app.crashes.mp4

Not sure tho if this is related to this bug or another bug.

@JaffaKetchup
Copy link
Member Author

Oooh, that's quite nasty, wonder if it's a memory leak or something, or whether the hover outlines are triggering it.

@JaffaKetchup
Copy link
Member Author

I can reproduce the increase in frame times, but not the crash (I assume that's device dependent). I'll try and do some more debugging.

image

@JaffaKetchup
Copy link
Member Author

This is unrelated to tap detection, as it appears to be present prior to the introduction of this. It is present in commit f31829f.
I think this is safe to be reviewed & merged.

@mohammedX6
Copy link

maybe just setting the shouldRepaint to true is the safest option now ?

@JaffaKetchup
Copy link
Member Author

Ah, the commit I mentioned above isn't necessarily the commit that caused it, just the previous one that I can reproduce the issue on.

Copy link
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright since it's not releated to this bug we can merge this pr. We should give the crash a high priority tho, maybe even create a bug report for it.

@JaffaKetchup JaffaKetchup merged commit 562bff1 into master Feb 14, 2024
8 checks passed
@JaffaKetchup JaffaKetchup deleted the fix-polygon-null-hitValue branch February 14, 2024 15:45
@JaffaKetchup
Copy link
Member Author

Report made in #1829.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants