Enable antialising for screenshots if enabled in user configuration#9363
Enable antialising for screenshots if enabled in user configuration#9363daniel-wer wants to merge 7 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/javascripts/viewer/view/rendering_utils.ts`:
- Line 101: The renderToTexture path is enabling MSAA unconditionally when
state.userConfiguration.antialiasRendering is true, which corrupts ID-picking;
add a new boolean parameter enableAntialias (default false) to the
renderToTexture function, change the MSAA line to only set renderTarget.samples
= 4 when enableAntialias && state.userConfiguration.antialiasRendering, and
update the screenshot export call (the caller at the screenshot/export path) to
pass enableAntialias = true; leave all other callers (e.g., ID-picking usages
from skeleton_handlers.ts and arbitrary_view.ts) unchanged so they continue to
call renderToTexture with the default (false).
In `@unreleased_changes/9363.md`:
- Line 2: Fix the typo in the changelog by replacing the misspelled word
"antialising" with "antialiasing" in the text of unreleased_changes/9363.md
(search for the exact token "antialising" to locate the line).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f446761f-d800-49c9-9ece-874b532237ef
📒 Files selected for processing (2)
frontend/javascripts/viewer/view/rendering_utils.tsunreleased_changes/9363.md
…x changelog typo.
philippotto
left a comment
There was a problem hiding this comment.
works well for me 👍 screenshot tests failed, though (havent looked into that).
Still, it's only used if users enabled antialiasing.
I wonder whether we should always enable this? I always disable anti-aliasing for performance reason. However, performance is mostly irrelevant for creating screenshots (instead, rendering quality is typically a priority). while comparing two screenshots (with and without AA), I only noticed pixel differences at geometries (the actual voxel data looked exactly the same). so, I don't see much harm in always enabling AA for screenshots. what do you think?
Makes a lot of sense 👍 |
|
The screenshot tests have been failing for a while 🙈 The changes look to be from the PR that adapted the node positioning to be in the voxel center. I'll have a look at regenerating them. |
|
@philippotto I recreated the screenshots on master and then ran the tests for this branch which was successful, so no changes due to this branch. I didn't spot any large differences looking at the diff in this PR, except for what looks like remnants of a toast at the upper right of the third viewport - do you know where that comes from? I remember having seen that before, but I don't recall the fix. |
|
On a closer look, for two datasets a white segment seems to have appeared in the top left center of the XY viewport on the new screenshots: I can confirm this via this link: https://antialiasedscreenshots.webknossos.xyz/annotations/69b9e4ff010000e10012a40d#512,1,0,0,0.047
|

Easy now that we use webgl 2. See https://discourse.threejs.org/t/why-is-a-custom-fbo-not-antialiased/1329/3 and https://github.com/mrdoob/three.js/pull/23455/changes#diff-5b25cb6d07a2d314fa35eb2138d99f2484837dcc079fd06e0cb108e8aa70618aR1983 (that's also where I got the samples=4 from).
This actually also fixed the colored borders of the viewports only showing up for two of the four sides for me 🤷 Still, it's only used if users enabled antialiasing.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)