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

Do not try to remove item if already deleted #1498

Merged
merged 2 commits into from
Sep 12, 2023
Merged

Conversation

roomrys
Copy link
Collaborator

@roomrys roomrys commented Sep 12, 2023

Description

In making some changes to the removal of overlays in #1421 (sub-PR: #1435), we reintroduced an error:

Traceback (most recent call last):
  File "C:\Users\lblab\.conda\envs\sleap\lib\site-packages\sleap\gui\app.py", line 1235, in _after_plot_change
    overlay.redraw(self.state["video"], frame_idx)
  File "C:\Users\lblab\.conda\envs\sleap\lib\site-packages\sleap\gui\overlays\base.py", line 74, in redraw
    self.remove_from_scene(*args, **kwargs)
  File "C:\Users\lblab\.conda\envs\sleap\lib\site-packages\sleap\gui\overlays\base.py", line 67, in remove_from_scene
    self.player.scene.removeItem(item)
RuntimeError: Internal C++ object (PySide2.QtWidgets.QGraphicsPathItem) already deleted.
Traceback (most recent call last):
  File "C:\Users\lblab\.conda\envs\sleap\lib\site-packages\sleap\gui\app.py", line 1235, in _after_plot_change
    overlay.redraw(self.state["video"], frame_idx)
  File "C:\Users\lblab\.conda\envs\sleap\lib\site-packages\sleap\gui\overlays\base.py", line 74, in redraw
    self.remove_from_scene(*args, **kwargs)
  File "C:\Users\lblab\.conda\envs\sleap\lib\site-packages\sleap\gui\overlays\base.py", line 67, in remove_from_scene
    self.player.scene.removeItem(item)
RuntimeError: Internal C++ object (PySide2.QtWidgets.QGraphicsPathItem) already deleted.

This is likely due to already removing the items in the overlay, but still keeping track of these items and then trying to remove them again. Even with this PR, if we enable DEBUG logging, then we will see that there are already deleted items trying to be removed.

This PR adds a try except around the item removal and removals all items from the tracked list of items after deleting them from the scene.

Types of changes

  • Bugfix
  • New feature
  • Refactor / Code style update (no logical changes)
  • Build / CI changes
  • Documentation Update
  • Other (explain)

Does this address any currently open issues?

[list open issues here]

Outside contributors checklist

  • Review the guidelines for contributing to this repository
  • Read and sign the CLA and add yourself to the authors list
  • Make sure you are making a pull request against the develop branch (not main). Also you should start your branch off develop
  • Add tests that prove your fix is effective or that your feature works
  • Add necessary documentation (if appropriate)

Thank you for contributing to SLEAP!

❤️

Summary by CodeRabbit


  • Bug Fix: Enhanced stability in the GUI by adding exception handling to prevent crashes when removing items from the scene.
  • Refactor: Updated the remove_from_scene method logic for better tracking of items, improving overall performance and consistency within the application.

@roomrys roomrys marked this pull request as ready for review September 12, 2023 21:05
@coderabbitai
Copy link

coderabbitai bot commented Sep 12, 2023

Walkthrough

This update enhances the robustness of the SLEAP GUI overlay system. It introduces better error handling when removing items from the scene, and improves tracking logic to ensure items are untracked post-removal.

Changes

File Summary
.../sleap/gui/overlays/base.py Introduced a logger for improved debugging. Added exception handling to manage potential RuntimeError during item removal from the scene. Updated remove_from_scene method to cease tracking of items once removed.

🐇💻

In the land of code where the shadows lie,
A rabbit hopped in, with a twinkle in its eye.
With a tap and a click, and a debug log,
He fixed the errors hidden in the fog.
Now the items vanish without a trace,
In the GUI's gentle embrace. 🎉


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between e4fca4f and 5313255 commits.
Files selected for processing (1)
  • sleap/gui/overlays/base.py (2 hunks)
Additional comments (Suppressed): 2
sleap/gui/overlays/base.py (2)
  • 12-25: The import statements have been rearranged and a new import for logging has been added. Ensure that this change doesn't break any dependencies in the code.

  • 66-78: This is a good improvement to handle the RuntimeError when trying to remove an item that has already been deleted. The use of logging to debug such issues is also a good practice.

-        for item in self.items:
-           self.player.scene.removeItem(item)
+        if self.items is None:
+            return
+        for item in self.items:
+            try:
+                self.player.scene.removeItem(item)
+            except RuntimeError as e:  # Internal C++ object (PySide2.QtWidgets.QGraphicsPathItem) already deleted.
+                logger.debug(e)
+                pass
+        # Stop tracking the items after they been removed from the scene
+        self.items = []

@roomrys roomrys merged commit b8a37c4 into develop Sep 12, 2023
5 checks passed
roomrys added a commit that referenced this pull request Sep 15, 2023
* Do not try to remove item if already deleted (#1498)

* Set `LD_LIBRARY_PATH` on `mamba activate` (#1496)

* Add version restrictions to tensorflow for pypi (#1485)

* Remove `imageio` pin (#1501)

* Reset LD_LIBRARY_PATH on deactivate (#1502)

* Brown bag bump to 1.3.3 (#1484)
roomrys added a commit that referenced this pull request Sep 18, 2023
@roomrys roomrys deleted the liezl/fix-overlay-delete branch October 6, 2023 21:50
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.

1 participant