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

Final shader applied on overlay #1984

Merged
merged 16 commits into from
Feb 26, 2025

Conversation

samoncrief
Copy link
Contributor

@samoncrief samoncrief commented Feb 13, 2025

@Meakk Here's the current version with linear blending applied. The code is all handled in vtkF3DOverlayRenderPass.

Fix for #1500

@mwestphal
Copy link
Contributor

Sorry I canceled your run, working on a CI issue right now

@mwestphal
Copy link
Contributor

I restarted CI :)

@mwestphal
Copy link
Contributor

LGTM but its more a @Meakk style of code.

@samoncrief samoncrief force-pushed the Final-shader-applied-on-overlay branch from 7bc731d to 4238d95 Compare February 18, 2025 18:48
@samoncrief samoncrief marked this pull request as ready for review February 18, 2025 20:06
@samoncrief samoncrief force-pushed the Final-shader-applied-on-overlay branch from b57a212 to 08f04d7 Compare February 21, 2025 20:09
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.75%. Comparing base (695141f) to head (54867a8).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1984      +/-   ##
==========================================
+ Coverage   95.69%   95.75%   +0.06%     
==========================================
  Files         126      128       +2     
  Lines       10219    10293      +74     
==========================================
+ Hits         9779     9856      +77     
+ Misses        440      437       -3     

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

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

LGTM appart the small codepath that is not reached by coverage

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

LGTM but this will need @Meakk review as well

@Meakk
Copy link
Member

Meakk commented Feb 24, 2025

Amazing work @samoncrief!
Can you add a new test with a final shader (e.g. negative shader) and a filename to properly test that the filename widget isn't affected?

@samoncrief
Copy link
Contributor Author

Amazing work @samoncrief! Can you add a new test with a final shader (e.g. negative shader) and a filename to properly test that the filename widget isn't affected?

Sure. Do you want it for a shader where alpha equals 1.0 like in the examples, or with the alpha values preserved? The example code treats the background as a solid object because of the alpha, so it may not give the intended results.

@samoncrief samoncrief force-pushed the Final-shader-applied-on-overlay branch from 5b38daf to f2fb61b Compare February 24, 2025 19:37
@Meakk
Copy link
Member

Meakk commented Feb 25, 2025

Do you want it for a shader where alpha equals 1.0 like in the examples, or with the alpha values preserved?

It's up to you :)

- set background when rendering overlay to handle linear blending
- incoming texture is not alpha premultiplied, so source does not divide by alpha before converting to linear space
- background is added back in after converting back to sRGB
@samoncrief samoncrief force-pushed the Final-shader-applied-on-overlay branch from ccffd13 to 9a59ba2 Compare February 25, 2025 17:56
@mwestphal mwestphal merged commit 13e81fa into f3d-app:master Feb 26, 2025
54 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.

3 participants