Skip to content

Add dataBounds() method to TextItem to make autoRange more predictable#2646

Merged
j9ac9k merged 4 commits intopyqtgraph:masterfrom
redruin1:TextItem-rendering-fix
Jun 21, 2023
Merged

Add dataBounds() method to TextItem to make autoRange more predictable#2646
j9ac9k merged 4 commits intopyqtgraph:masterfrom
redruin1:TextItem-rendering-fix

Conversation

@redruin1
Copy link
Copy Markdown
Contributor

@redruin1 redruin1 commented Mar 9, 2023

Summary

Sacrifices some visual polish to make the autorange functionality with TextItems much more predictable. "Fixes" #2642, though theoretically with some more effort it should be possible to still get high visual fidelity without runaway scaling.

Details

Adds the dataBounds() method to TextItem, which supersedes the result from boundingRect() when updating the ViewBox's autorange. Instead, only the anchor point is returned, behaving similarly to how a set of data points on a graph would be scaled to.

Benefits

This PR fixes:

  • Runaway scaling when there is only one TextItem in a PlotWidget

    Example
    import pyqtgraph as pg

    pg.mkQApp()
    pw = pg.PlotWidget()

    text_item = pg.TextItem("Any text")
    pw.addItem(text_item)

    pw.show()
    pg.exec()

  • Runaway scaling when there is multiple TextItems in a PlotWidget, but they all share the same axis

    Example
    import pyqtgraph as pg

    pg.mkQApp()
    pw = pg.PlotWidget()

    text_item = pg.TextItem("Any text")
    pw.addItem(text_item)

    text_item = pg.TextItem("Some other text")
    text_item.setPos(10, 0)
    pw.addItem(text_item)

    pw.show()
    pg.exec()

  • Runaway scaling when the visual length of a TextItem cannot entirely fit within a ViewBox at the current resolution/font size

    Example
    import pyqtgraph as pg

    pg.mkQApp()
    pw = pg.PlotWidget()

    pw.plot([1, 2, 3], [4, 5, 6])

    text = 'A' * 1000
    text_item = pg.TextItem(text)
    pw.addItem(text_item)

    pw.show()
    pg.exec()

  • Runaway scaling with TextItem as a whole should be largely eliminated

Downsides

  • TextItems are no longer guaranteed to be entirely (or even partially) visible, which reduces the visual quality under non-edge cases:

image

Files that need updates

Details
  • README.md (None needed)
  • setup.py (None needed)
  • tox.ini (None needed)
  • .github/workflows/main.yml and associated requirements.txt and conda environemt.yml files (None needed)
  • pyproject.toml (None needed)
  • binder/requirements.txt (None needed)
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

@redruin1
Add dataBounds() method to TextItem
10d4805 
Sacrifices some visual polish to make the autorange functionality with TextItems much more predictable.
@campagnola
Copy link
Copy Markdown
Member

campagnola commented Jun 16, 2023

Looks good to me! I am ok with changing the default behavior here, but can you also add an init argument to disable dataBounds and restore the original behavior?

@redruin1
Copy link
Copy Markdown
Contributor Author

redruin1 commented Jun 16, 2023

@campagnola Sure, no problem. Any preferences for what that init argument would be called? (So I can be in line with the rest of the API)

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 17, 2023

Hi @redruin1

First off, I owe you an apology for letting this PR sit for so long without a response, that wasn't fair to you. This is a very high quality PR that does a fantastic job highlighting the issue, the fix and potential downside. Thank you for that.

I'm also good w/ changing the default behavior; I'm not sure we need to keep an argument to preserve legacy behavior tho (but @campagnola being the original author outranks me :D ).

I don't think there is an argument in pyqtgraph that we can use for to maintain consistency, but there might be something in the Qt API. Scrolling through the Qt API looks like perhaps a flag is what we would want to add, and while this topic has come up before, we've never actually implemented out our enums that can be used as flags. Maybe ensureInViewport with default to False, and in the docstring referencing this PR (or associated issue).

@redruin1
added init param ensureInBounds
331cd8c 
also updated clause in ViewBox to handle `None` case
@redruin1
Copy link
Copy Markdown
Contributor Author

redruin1 commented Jun 17, 2023

Alright, this was the least intrusive way to get the desired behavior that I could come up with. Ideally there would probably be some inheritance shuffling to enable/disable the dataBounds method properly, but for now all I've done is conditionally set the attribute to None in TextItem.__init__():

    if ensureInBounds:
        self.dataBounds = None # Doesn't like del or delattr here, which is probably for the best

The only other change (aside from docs) is changing the criteria in ViewBox:

if hasattr(item, 'dataBounds') and item.dataBounds is not None
    ...

This could maybe be more strict in checking that item.dataBounds is a Callable object, but I doubt this will be used frequently elsewhere. Tests pass on my machine.

I didn't make helper methods for setting/modifying ensureInBounds as it would be complex to maintain and ensure that it works properly, and it seems more proper as just a init parameter. That being said, would you like it to be an accessible attribute so that users can query whether or not the TextItem is guaranteed to be fully visible?

@redruin1
Copy link
Copy Markdown
Contributor Author

redruin1 commented Jun 17, 2023 •
edited
Loading

Hmm, seems like this interferes with the OpenGL glGetError wrapper function also not expecting None in place of a method. Should I investigate a more comprehensive solution?

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 17, 2023

I think you're on the right track, let me take a look later today and I can see if I come up with something.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 18, 2023 •
edited
Loading

oh, the glError in read the docs CI is entirely unrelated, if you rebase master, that should go away (I should have fixed this a few weeks back)

The errors on python 3.11 with PyQt6 are due to a cpython bug here: python/cpython#105497

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 19, 2023

should the file to_delete.py be removed from the PR?

@redruin1
Copy link
Copy Markdown
Contributor Author

redruin1 commented Jun 20, 2023

whoops

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jun 21, 2023

Thanks for the PR @redruin1 and thanks for your patience with a review. Merging!

@j9ac9k j9ac9k merged commit 843541c into pyqtgraph:master Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@redruin1 @campagnola @j9ac9k