Skip to content

Improve 2D heuristic fallback values for image opacity #9787

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Apr 23, 2025

Previously, only segmentation images had an opacity fallback of 0.5 when other non-segmentation images were in the same view. We now keep track of the number of images per image kind, and use that to determine what the opacity should be for all image kinds that support opacity.

The images further to the front will be transparent if there are images possibly behind them.

Assuming the draw order fallback is also used, the draw order will be (front to back):

  • Segmentation
  • Color
  • Depth (does not support opacity, so we can ignore that)

Examples of fallback opacity:

  • 2x Segmentation:
    • Segmentation (1): 1.0
    • Segmentation (2): 1.0
  • 1x Segmentation + 1x Color:
    • Segmentation: 0.5
    • Color: 1.0
  • 1x Color + 1x Depth:
    • Color: 0.5
    • Depth: 1.0 (ignored)

@jprochazk jprochazk added the exclude from changelog PRs with this won't show up in CHANGELOG.md label Apr 23, 2025
Copy link

github-actions bot commented Apr 23, 2025

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
22c819d https://rerun.io/viewer/pr/9787 +nightly +main

Note: This comment is updated whenever you push a commit.

@jprochazk
Copy link
Member Author

So, this is annoying. I noticed some snapshot tests were failing, and after updating them, now they're failing on CI. But looking at the differences, there really aren't any. Not sure what to do here.

@emilk emilk requested a review from Wumpf April 23, 2025 14:51
@@ -205,8 +207,21 @@ impl EncodedImageVisualizer {
}

impl TypedComponentFallbackProvider<Opacity> for EncodedImageVisualizer {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be the same for VideoVisualizer then

@Wumpf Wumpf changed the title Image opacity heuristic Improve 2D heuristic fallback values for image opacity Apr 29, 2025
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

lgtm!

Emil is right that we should also add this to the video visualizer just the same! Please do so before merging, thx!

The image comparision changes are strange indeed! Have you checked the diff images that were changed on ci? Ci generates a zip with before/after/diff images

Is it maybe an LFS issue (before/after not being lfs)? (doesn't look like though, it says 4 lines changed for the diff)

@jprochazk
Copy link
Member Author

Have you checked the diff images that were changed on ci?

Yes. There is some difference between how it renders on my machine and CI. Is there a chance that it's different llvmpipe versions, or maybe it isn't software rendering on my end?

@Wumpf
Copy link
Member

Wumpf commented Apr 29, 2025

No change that I'm aware of, no
But yeah we don't do software rendering on dev machines right now since the setup is a bit tedious/invasive and depends on platform circumstances

I'll have a poke later today!

@jprochazk
Copy link
Member Author

Emil is right that we should also add this to the video visualizer just the same! Please do so before merging, thx!

It looks like video doesn't support Opacity. At a glance it seems like it could, but it feels out of scope for this PR

@jprochazk
Copy link
Member Author

Opened an issue for that and linked it in a comment:

@jprochazk
Copy link
Member Author

Decided to revert the snapshot to what's on CI for the time being

@jprochazk
Copy link
Member Author

which is proving to be a mistake, because now I'll have to wait for CI to report all the sequential failures one by one...

@Wumpf
Copy link
Member

Wumpf commented Apr 29, 2025

Opened an issue for that and linked it in a comment:

* [Support `Opacity` for `VideoFrameReference` #9832](https://github.com/rerun-io/rerun/issues/9832)

thanks!

@Wumpf Wumpf self-requested a review April 29, 2025 15:22
@jprochazk jprochazk added the do-not-merge Do not merge this PR label Apr 29, 2025
@jprochazk
Copy link
Member Author

jprochazk commented Apr 29, 2025

don't merge this yet, i left some temporary code in tests so i could get more failures in parallel

(it was supposed to be temporary, but i think it's fine to leave it in)

Copy link

github-actions bot commented Apr 29, 2025

Latest documentation preview deployed successfully.

Result Commit Link
22c819d https://landing-e8m7hrx5k-rerun.vercel.app/docs

Note: This comment is updated whenever you push a commit.

@jprochazk
Copy link
Member Author

jprochazk commented Apr 29, 2025

It seems like what you pushed in b914586 didn't fix the failures 🤔

(my fault, forgot to invert the condition)

@jprochazk jprochazk removed the do-not-merge Do not merge this PR label Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants