Skip to content

Remove keybinds that didn't work, rebind screenshot - #477

Merged
alessandrofelder merged 1 commit into
mainfrom
fix/close-scenes
Oct 13, 2026
Merged

alessandrofelder merged 1 commit into
mainfrom
fix/close-scenes

Conversation

@IgorTatarnikov

Copy link
Copy Markdown
Member

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
CI was having errors due to garbage collection of Scene objects. They were being kept alive by a circular reference between Scene and Plotter. The culprit can be seen below, this didn't have any effect but kept a reference hanging around preventing collection.

self.plotter.keyPressFunction = self.keypress

What does this PR do?
Removes the previous machinery for adding key bindings that had no effect (tested by trying to use s for screenshot in a scene, only Shift+s worked for the vedo screenshot). Added the screenshot key bind back using a weak ref this time to prevent the same loop.

Now:
q quits as before
s calls the brainrender.Scene.screenshot method
Shift + c prints the camera information (previously this was stated to be c but that wasn't actually bound, this was perhaps the old vedo key bind that was relied on implicitly)

How has this PR been tested?

Tested locally, the new screenshot key bind works.

Is this a breaking change?

No

Does this PR require an update to the documentation?

We should make sure the key bindings are documented somewhere.

Checklist:

  • The code has been tested locally
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.59%. Comparing base (0519356) to head (afc5b3f).

Files with missing lines Patch % Lines
brainrender/render.py 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #477      +/-   ##
==========================================
+ Coverage   88.55%   89.59%   +1.04%     
==========================================
  Files          27       27              
  Lines        1319     1326       +7     
==========================================
+ Hits         1168     1188      +20     
+ Misses        151      138      -13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alessandrofelder alessandrofelder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alessandrofelder
alessandrofelder merged commit b323076 into main Oct 13, 2026
20 of 21 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.

2 participants