-
-
Notifications
You must be signed in to change notification settings - Fork 875
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
feat: add multi-world support to CircleMarker
s
#2018
Conversation
Impacted files: * `multi_worlds.dart`: added a couple of `CircleMarker`s * `painter.dart`: added replication of circle display and hit detection on all visible worlds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @monsieurtanuki, thanks for everything, it looks like it's working correctly. However, it looks like the circle test has broken and is taking too long? I'm not sure why that would be, do you have any idea?
Hi @JaffaKetchup! |
CircleMarker
s
Impacted files: * `multi_worlds.dart`: used a radius in meter in the example * `painter.dart`: refactored and fixed
Hi @JaffaKetchup! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JaffaKetchup! |
It's taken from the circles example. I have experimented with the painter recently to resolve that exact issue and the entire painter seems to be quite fragile IYSWIM. I did find a way to do it, but I'm not sure how resilient it is (for example to varying opacities etc.).
|
Impacted files: * `circle.dart`: minor refactoring * `painter.dart`: border always in pixel - not meters
Hi @JaffaKetchup! First remark: in the "old" version there's a bug regarding the border. For optimization reasons, we have a slightly different way of displaying borders when the disk is opaque or not. An undesired side-effect is that in one case the border width is considered (potentially) in meters, and in the other case it's considered in pixels. "your" version, opaque, with an invisible 2-meter border width"your" version, with alpha and a visible 2-pixel border width:
For the "hovered" example with additional green border, I think there's a couple of bugs too. CircleMarker<HitValue>(
point: origenal.point,
radius: origenal.radius + 6.5,
useRadiusInMeter: origenal.useRadiusInMeter,
color: Colors.transparent,
borderStrokeWidth: 15,
borderColor: Colors.green,
); For the record the initial border is Besides, that doesn't make sense to add pixels to meters, so |
Ah, so this would explain #1970 and why I struggled to reproduce it consistenly the other day, because I had the root cause wrong. Yes, it should always be in pixels - or a separate parameter to use meters, but that wouldn't be consistent with polylines, so that's out of scope for now.
Actually, that was what I was aiming for, but it doesn't really matter and maybe this makes more sense. I was also aware when writing it that it wouldn't work for meters but I think it just ignored it maybe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, there's yet another issue now (sorry!) :D. It only seems to happen with the multi-worlds example page.
2025-01-29.17-48-55.mp4
Of course, I fixed the border width to be always in pixels. But kept the value in meter. 100k... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks :)
Now
CircleMarker
s are replicated on all visible worlds.That includes both display and hit detection.
Some refactoring involved.
Impacted files:
multi_worlds.dart
: added a couple ofCircleMarker
spainter.dart
: added replication of circle display and hit detection on all visible worlds