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 BlendMode support to CircleLayer #1869

Closed
wants to merge 3 commits into from

Conversation

Y0ngg4n
Copy link

@Y0ngg4n Y0ngg4n commented Apr 11, 2024

Currently overlapping Polygons are adding up in stroke color. To fix this it adds the BlendMode.src to all so this should not happen anymore.
Before:

image

@josxha josxha added this to the v7.0 milestone Apr 11, 2024
Copy link
Collaborator

@mootw mootw left a comment

Choose a reason for hiding this comment

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

do you have an example after picture or some code to test with? Also any particular reason to hard code the BlendMode vs pass it in as a parameter to the layer? Thanks for your contribution!

@Y0ngg4n
Copy link
Author

Y0ngg4n commented Apr 12, 2024

@mootw i could not test it because of dependency hell. But it uses the same blend mode as in the polygons so the behaviour should be the same.

@Y0ngg4n
Copy link
Author

Y0ngg4n commented Apr 12, 2024

@mootw then i guess it would be better to also parametrize it in the polygons

@@ -92,6 +93,7 @@ class CirclePainter extends CustomPainter {
for (final radius in pointsByRadius.keys) {
final pointsByRadiusColor = pointsByRadius[radius]!;
final radiusPaint = paint..strokeWidth = radius * 2;
radiusPaint.blendMode = BlendMode.src;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move it to first assignment (paintPoint) to avoid useless re-assignment.

@@ -103,6 +105,7 @@ class CirclePainter extends CustomPainter {
for (final radius in pointsByRadius.keys) {
final pointsByRadiusColor = pointsByRadius[radius]!;
final radiusPaint = paint..strokeWidth = radius * 2;
radiusPaint.blendMode = BlendMode.src;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove it if previous comment resolved.

@@ -73,6 +73,7 @@ class CirclePainter extends CustomPainter {
for (final borderWidth in pointsBorder[color]!.keys) {
final pointsByRadius = pointsBorder[color]![borderWidth]!;
final radiusPaint = paint..strokeWidth = borderWidth;
radiusPaint.blendMode = BlendMode.src;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move it to first assignment (paintBorder) to avoid useless re-assignment.

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 @Y0ngg4n, thanks for this so far!

Some changes do need to be made before we can merge this though:

  • We will require an example (or an update to the exisiting one), or at least some code to test on this occassion
  • It may also be a good idea to incorporate this into the polygon and polyline layers, although they use some more complex logic, so it may not work very performantly (may require more calls to saveLayer). This may be out of the scope of this PR?
  • And as said, it should become a property on both the CircleLayer and CircleMarker, with the same decision logic as other properties.

@JaffaKetchup JaffaKetchup changed the title Add BlendMode src like in Polygons to make overlapping circles possible feat: add BlendMode support to CircleLayer Apr 17, 2024
@Y0ngg4n
Copy link
Author

Y0ngg4n commented Apr 17, 2024

@JaffaKetchup thank you for your answer.

  • It can take a little bit until i have the time to write an example as i am currently limited in time
  • For polygons it already works, as they already use the right BlendMode
  • Then it should also become a property for the polygons too

@JaffaKetchup JaffaKetchup marked this pull request as draft May 15, 2024 12:39
@JaffaKetchup
Copy link
Member

I'm going to close this for now. This is a good option to introduce to all the painting layers, but that needs a new PR.

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

Successfully merging this pull request may close these issues.

5 participants