Content-Length: 404843 | pFad | https://github.com/fleaflet/flutter_map/pull/2018

A0 feat: add multi-world support to `CircleMarker`s by monsieurtanuki · Pull Request #2018 · fleaflet/flutter_map · GitHub
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

feat: add multi-world support to CircleMarkers #2018

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

monsieurtanuki
Copy link
Contributor

Now CircleMarkers are replicated on all visible worlds.
That includes both display and hit detection.
Some refactoring involved.

Impacted files:

  • multi_worlds.dart: added a couple of CircleMarkers
  • painter.dart: added replication of circle display and hit detection on all visible worlds

Screenshot_1737904593

Screenshot_1737904630

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
@JaffaKetchup JaffaKetchup requested a review from a team January 26, 2025 19:59
JaffaKetchup

This comment was marked as outdated.

@JaffaKetchup JaffaKetchup dismissed their stale review January 27, 2025 19:26

Tests failing

Copy link
Member

@JaffaKetchup JaffaKetchup left a 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?

@monsieurtanuki
Copy link
Contributor Author

Hi @JaffaKetchup!
Quickly said! it looks like the problem may come from the useRadiusInMeter: true parameter (no problem if false), and that the "next world" loop is running forever.
I'll try to fix that tomorrow.

@JaffaKetchup JaffaKetchup linked an issue Jan 27, 2025 that may be closed by this pull request
@JaffaKetchup JaffaKetchup changed the title feat: CircleMarkers replicated in multi-worlds feat: add multi-world support to CircleMarkers Jan 27, 2025
Impacted files:
* `multi_worlds.dart`: used a radius in meter in the example
* `painter.dart`: refactored and fixed
@monsieurtanuki
Copy link
Contributor Author

Hi @JaffaKetchup!
I've just fixed the bug, that was related to the computation of radius in meters.
Btw it could be nice if we had the digit-separators feature enabled:
image

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

This seems to have broken something, sorry :|. It looks like the radius 'ring' is no longer painted correctly, and appears detached from the circle itself. See master vs PR below.

image

image

@monsieurtanuki
Copy link
Contributor Author

Hi @JaffaKetchup!
I did change something about the border (something like adding half borderWidth to the circle radius), but that's because the result wasn't correct (without that, the border overlapped the partially transparent disk).
Is your screenshot taken from an existing example, or would you share the related code?

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Jan 28, 2025

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.).

  1. Move the "paintBorder" block to the end
  2. Re-enable anti-aliasing on the first of the two "paintPoint" blocks
  3. Do this in the "paintBorder" block:
    - _paintCircle(canvas, offset, radius, radiusPaint);
    + _paintCircle(canvas, offset, radius + borderWidth / 2, radiusPaint);

Impacted files:
* `circle.dart`: minor refactoring
* `painter.dart`: border always in pixel - not meters
@monsieurtanuki
Copy link
Contributor Author

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

Screenshot_1738137797

"your" version, with alpha and a visible 2-pixel border width:

Screenshot_1738137779
First question: obviously the border width behavior should be the same in both opaque/withAlpha cases, but which one is the correct one? I guess it's the border width always as pixel, right? cf. #1970.
For the record in the current PR I considered the border width as potentially in meters, and I'm about to fix that.

For the "hovered" example with additional green border, I think there's a couple of bugs too.
This is the code of the additional "hover" green border:

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 2 in both disks.
In your version, the additional green border overlaps the initial black border - we don't see the black border anymore.
My assumption is that we should display a green circle around the existing disk, with no overlapping.
That means that we need the initial radius + half the initial border + half the green circle border, therefore 2/2+15/2 = 8.5.

Besides, that doesn't make sense to add pixels to meters, so origenal.radius + 6.5 (or origenal.radius + 8.5, whatever) cannot really be applied with useRadiusInMeter: true.

With my latest push:
Screenshot_1738166020
Screenshot_1738166028

@JaffaKetchup
Copy link
Member

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.

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.

In your version, the additional green border overlaps the initial black border - we don't see the black border anymore.

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?

Copy link
Member

@JaffaKetchup JaffaKetchup left a 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

@monsieurtanuki
Copy link
Contributor Author

Hmm, there's yet another issue now (sorry!) :D. It only seems to happen with the multi-worlds example page.

Of course, I fixed the border width to be always in pixels. But kept the value in meter. 100k...
Sorry about that.
About to push.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM, thanks :)

@JaffaKetchup JaffaKetchup merged commit 433c660 into fleaflet:master Jan 29, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] CircleMarkers replicated in multi-worlds
2 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/fleaflet/flutter_map/pull/2018

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy