Skip to content

Final shader improvements (reopened)#2932

Merged
mwestphal merged 9 commits into
f3d-app:masterfrom
RealEdwardS:final-shader-improvements
Apr 6, 2026
Merged

Final shader improvements (reopened)#2932
mwestphal merged 9 commits into
f3d-app:masterfrom
RealEdwardS:final-shader-improvements

Conversation

@RealEdwardS

Copy link
Copy Markdown
Contributor

Describe your changes

Added time elapsed from the app start as a uniform property for final shader application.

Issue ticket number and link if any

Checklist for finalizing the PR

  • I have performed a self-review of my code
  • I have added tests for new features and bugfixes
  • I have added documentation for new features
  • If it is a modifying the libf3d API, I have updated bindings
  • If it is a modifying the .github/workflows/versions.json, I have updated docker_timestamp

Continuous integration

Please write a comment to run CI, eg: \ci fast.
See here for more info.

@github-actions

github-actions Bot commented Mar 11, 2026

Copy link
Copy Markdown

You are modifying libf3d public API! ⚠️Please update bindings accordingly⚠️!
You can find them in their respective directories: c, python, java, webassembly.

@mwestphal

Copy link
Copy Markdown
Member

Still not ok ^^

@RealEdwardS RealEdwardS force-pushed the final-shader-improvements branch from 0de55a3 to 9ab0a67 Compare March 11, 2026 19:47
@mwestphal

Copy link
Copy Markdown
Member

I can take care of it if you want.

@RealEdwardS

Copy link
Copy Markdown
Contributor Author

I can take care of it if you want.

Yes please

@mwestphal

Copy link
Copy Markdown
Member

here it is: #2935

Please next time you want to merge/rebase on master, please reach out on discord and I will assist.

@mwestphal

Copy link
Copy Markdown
Member

Still working on this @RealEdwardS ?

@RealEdwardS

Copy link
Copy Markdown
Contributor Author

Still working on this @RealEdwardS ?

I think it's ready for review, unless there's something I'm missing?

@mwestphal mwestphal self-requested a review March 23, 2026 10:06
@mwestphal

Copy link
Copy Markdown
Member

I think it's ready for review, unless there's something I'm missing?

You need to ask for review if you want a review :)

@mwestphal

mwestphal commented Mar 23, 2026

Copy link
Copy Markdown
Member

Well, first you have to recover the branch I provided to you here: #2935, do you need help with that ?

@RealEdwardS RealEdwardS force-pushed the final-shader-improvements branch 7 times, most recently from 90c3272 to 58d6baa Compare March 24, 2026 01:18
@RealEdwardS

Copy link
Copy Markdown
Contributor Author

Well, first you have to recover the branch I provided to you here: #2935, do you need help with that ?

I think I was able to fix everything?

@RealEdwardS RealEdwardS force-pushed the final-shader-improvements branch from 58d6baa to 5c2722c Compare March 24, 2026 02:19
Comment thread vtkext/private/module/vtkF3DUserRenderPass.cxx Outdated
Comment thread vtkext/private/module/vtkF3DUserRenderPass.cxx Outdated
Comment thread application/testing/tests.features.cmake
Comment thread vtkext/private/module/vtkF3DRenderer.h
Comment thread vtkext/private/module/vtkF3DUserRenderPass.cxx Outdated
Comment thread vtkext/private/module/vtkF3DUserRenderPass.cxx

@mwestphal mwestphal 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.

changes needed, but a review from meak too.

@mwestphal mwestphal requested a review from Meakk March 24, 2026 07:03

@Meakk Meakk 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.

The implementation and test is fine. OK to merge when the discussions are addressed.

@snoyer

snoyer commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Do we know if/how that plays out in non-interactive mode, like rendering a video frame by frame? Would we be able to set the time variable with an API in the future?

Comment thread vtkext/private/module/vtkF3DUserRenderPass.cxx Outdated
@RealEdwardS RealEdwardS force-pushed the final-shader-improvements branch from e83857f to 7837916 Compare April 5, 2026 14:59
@RealEdwardS

Copy link
Copy Markdown
Contributor Author

@Meakk, @mwestphal. I think it's ready for review :D

@mwestphal

Copy link
Copy Markdown
Member

@Meakk, @mwestphal. I think it's ready for review :D

please use review mechanism top right.

@RealEdwardS

Copy link
Copy Markdown
Contributor Author

What’s the review mechanism?

@RealEdwardS RealEdwardS requested a review from mwestphal April 5, 2026 16:45
@mwestphal

Copy link
Copy Markdown
Member

What’s the review mechanism?

You just found it :)

Comment thread vtkext/private/module/vtkF3DUserRenderPass.cxx

@mwestphal mwestphal 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.

LGTM but I defer to @Meakk

@mwestphal mwestphal requested a review from Meakk April 5, 2026 17:00
@Meakk Meakk added ci:full and removed ci:fast labels Apr 6, 2026
@Meakk

Meakk commented Apr 6, 2026

Copy link
Copy Markdown
Member

Looks good :)
I've run the full CI and will merge it if green

@codecov

codecov Bot commented Apr 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.17%. Comparing base (f7bf2e2) to head (7837916).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2932   +/-   ##
=======================================
  Coverage   97.17%   97.17%           
=======================================
  Files         210      210           
  Lines       16729    16733    +4     
=======================================
+ Hits        16256    16260    +4     
  Misses        473      473           

☔ View full report in Codecov by Sentry.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mwestphal

Copy link
Copy Markdown
Member

CI is green, merging.

@mwestphal mwestphal merged commit 8b2e97f into f3d-app:master Apr 6, 2026
88 checks passed
@mwestphal

Copy link
Copy Markdown
Member

Thanks for the contribution @RealEdwardS !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants