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

Filling correction for Fillbetweenitem #2971

Merged
merged 3 commits into from Apr 18, 2024

Conversation

BousquetSophie
Copy link
Contributor

Detail the reasoning behind the code change. If there is an associated issue that this PR will resolve add

Fixes #1698 When we have 2 intersecting polygons, there's no more fill at the intersection.

Capture d’écran du 2024-03-26 10-22-43

So, we've extracted the intersection points and used them to create a new polygon to fill the empty space.

Capture d’écran du 2024-03-26 10-27-04

Other Tasks

Bump Dependency Versions
  • ubuntu 22.04

  • python 3.10.12

  • Pyqtgraph 0.13.4.dev0

Files that need updates

Confirm the following files have been either updated or there has been a determination that no update is needed.

  • README.md
  • setup.py
  • tox.ini
  • .github/workflows/main.yml and associated requirements.txt and conda environemt.yml files
  • pyproject.toml
  • binder/requirements.txt
Pre-Release Checklist

Pre Release Checklist

  • Update version info in __init__.py
  • Update CHANGELOG primarily using contents from automated changelog generation in GitHub release page
  • Have git tag in the format of pyqtgraph-
Post-Release Checklist

Steps To Complete

  • Append .dev0 to __version__ in __init__.py
  • Announce on mail list
  • Announce on Twitter

@BousquetSophie BousquetSophie changed the title Fillbetweenitem issue1698 Filling correction for Fillbetweenitem Mar 26, 2024
Copy link
Member

@j9ac9k j9ac9k left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @BousquetSophie ! FillBetweenItem doesn't get as much attention as it should.

The only comment I had is regarding the use of lineTo; I won't let that be a deal breaker, but if you could implement a fix without having moveTo/lineTo that would be preferred.

pyqtgraph/graphicsItems/FillBetweenItem.py Outdated Show resolved Hide resolved
@j9ac9k
Copy link
Member

j9ac9k commented Apr 18, 2024

Thanks for the PR @BousquetSophie Merging!

@j9ac9k j9ac9k merged commit 84f9a54 into pyqtgraph:master Apr 18, 2024
29 checks passed
@jmakovicka
Copy link
Contributor

Hi, this started causing problems for us when filling between two non-convex curves that are not closed (basically, an area chart) . The intersected() function closes the curves and creates an additional undesired area.

@jmakovicka
Copy link
Contributor

Would it be sufficient to restrict this logic to closed polygons?

diff --git a/pyqtgraph/graphicsItems/FillBetweenItem.py b/pyqtgraph/graphicsItems/FillBetweenItem.py
index 30be3092..a702bab2 100644
--- a/pyqtgraph/graphicsItems/FillBetweenItem.py
+++ b/pyqtgraph/graphicsItems/FillBetweenItem.py
@@ -78,8 +78,9 @@ class FillBetweenItem(QtWidgets.QGraphicsPathItem):
             return
         
         for p1, p2 in zip(ps1, ps2):
-            intersection = p1.intersected(p2)
-            if not intersection.isEmpty():
-                path.addPolygon(intersection)
+            if not p1.isEmpty() and not p2.isEmpty() and p1[0] == p1[-1] and p2[0] == p2[-1]:
+                intersection = p1.intersected(p2)
+                if not intersection.isEmpty():
+                    path.addPolygon(intersection)
             path.addPolygon(p1 + p2)       
         self.setPath(path)

@j9ac9k
Copy link
Member

j9ac9k commented Apr 23, 2024

Hi, this started causing problems for us when filling between two non-convex curves that are not closed (basically, an area chart) . The intersected() function closes the curves and creates an additional undesired area.

Sorry about that! I think restricting to closed polygons would indeed be a sufficient solution.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 27, 2024

Would it be sufficient to restrict this logic to closed polygons?

diff --git a/pyqtgraph/graphicsItems/FillBetweenItem.py b/pyqtgraph/graphicsItems/FillBetweenItem.py
index 30be3092..a702bab2 100644
--- a/pyqtgraph/graphicsItems/FillBetweenItem.py
+++ b/pyqtgraph/graphicsItems/FillBetweenItem.py
@@ -78,8 +78,9 @@ class FillBetweenItem(QtWidgets.QGraphicsPathItem):
             return
         
         for p1, p2 in zip(ps1, ps2):
-            intersection = p1.intersected(p2)
-            if not intersection.isEmpty():
-                path.addPolygon(intersection)
+            if not p1.isEmpty() and not p2.isEmpty() and p1[0] == p1[-1] and p2[0] == p2[-1]:
+                intersection = p1.intersected(p2)
+                if not intersection.isEmpty():
+                    path.addPolygon(intersection)
             path.addPolygon(p1 + p2)       
         self.setPath(path)

Can you verify that #3006 fixes your issue?

@jmakovicka
Copy link
Contributor

Can you verify that #3006 fixes your issue?

Sure, that one works too.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 28, 2024

Can you verify that #3006 fixes your issue?

Sure, that one works too.

Hey, sorry to ask you again, would you mind trying that branch again? I just made another update to it (that will better satisfy #1698) but it may have some downstream consequences.

If FillBetweenItem is working as expected, I'll likely do another release with the FillBetweenItem regression fix.

@jmakovicka
Copy link
Contributor

This one breaks it again for us, this time producing a stray line crossing the whole chart.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 28, 2024

This one breaks it again for us, this time producing a stray line crossing the whole chart.

Really appreciate you checking, if you have an example I can run (doesn't have to be a minimum reproducible example), I'll gladly check that while trying to come up w/ a solution to #1698 so I don't have to bug you each time I try something new 😆

@jmakovicka
Copy link
Contributor

Sure, I believe this one illustrates the problems well

import numpy as np

import pyqtgraph as pg
from pyqtgraph.Qt import QtCore

win = pg.plot()
win.setWindowTitle('pyqtgraph example: FillBetweenItem')
win.setXRange(-1, 6)
win.setYRange(-1, 6)

x = np.linspace(0, 5, 5)

y1 = [2,3,5,2,1]
y2 = [1,2,4,1,0]

d1 = pg.PlotDataItem(x, y1)
d2 = pg.PlotDataItem(x, y2)

fb = pg.FillBetweenItem(d1, d2, brush='w')

win.addItem(fb)

if __name__ == '__main__':
    pg.exec()

@j9ac9k
Copy link
Member

j9ac9k commented Apr 28, 2024

I just got this image when running this example code, I'm not seeing a stray line, maybe this is platform dependent?

image

@jmakovicka
Copy link
Contributor

The simplified example looks different, but the expected output is definitely this:
image

@jmakovicka
Copy link
Contributor

jmakovicka commented Apr 28, 2024

In the original, the "stray line" is in fact the two triangles at the bottom, just stretched.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 28, 2024

ahh god it, I'm being daft.. my apologies. Alright, I'm getting to work 👍🏻

j9ac9k added a commit that referenced this pull request Apr 29, 2024
Allow users to specify FillRule for FillBetweenItem, undo regressionfrom #2971
@j9ac9k
Copy link
Member

j9ac9k commented Apr 29, 2024

@jmakovicka Released 0.13.7, which includes a fix to the regression I shipped. Also added a fillRule optional arg and updated the docstrings (can you really say they were an update if there were hardly any to begin with?).

Thanks for your prompt report on the breakage.

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.

FillBetweenItem has no way to change FillRule
3 participants