Skip to content

Conversation

@cwfitzgerald
Copy link

Objective

Solution

  • Describe the solution used to achieve the objective above.

Testing

  • Did you test these changes? If so, how?
  • Are there any parts that need more testing?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Showcase

This section is optional. If this PR does not include a visual change or does not add a new feature, you can delete this section.

  • Help others understand the result of this PR by showcasing your awesome work!
  • If this PR adds a new feature or public API, consider adding a brief pseudo-code snippet of it in action
  • If this PR includes a visual change, consider adding a screenshot, GIF, or video
    • If you want, you could even include a before/after comparison!
  • If the Migration Guide adequately covers the changes, you can delete this section

While a showcase should aim to be brief and digestible, you can use a toggleable section to save space on longer showcases:

Click to view showcase
println!("My super cool code.");

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@github-actions
Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-21669

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

camera_fov_y,
camera_near,
camera_far,
frame_time_delta: time.delta_secs() * 1000.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

why * 1000.0?

Copy link
Author

Choose a reason for hiding this comment

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

Milliseconds

>,
) {
#[cfg(all(feature = "dlss", not(feature = "force_disable_dlss")))]
let (projection, fxaa, smaa, taa, cas, msaa, dlss) = *camera;
Copy link
Contributor

Choose a reason for hiding this comment

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

probably want fsr in both code paths, otherwise dlss would kind of be a negative feature?

MipBias,
DepthPrepass,
MotionVectorPrepass,
Fsr3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Move Fsr3 to the front of the list

MotionBlur,
Taa,
Fsr3,
DlssSuperResolution,
Copy link
Contributor

Choose a reason for hiding this comment

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

Idk if we need a generic TSR label or something long term, but for now this is fine.

//! AMD FidelityFX Super Resolution 3 (FSR3).
//!
//! FSR3 is a temporal upscaling and anti-aliasing technique that uses
//! machine learning-based upscaling to render at a lower resolution
Copy link
Contributor

Choose a reason for hiding this comment

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

Err it doesn't use ML?

Copy link
Author

Choose a reason for hiding this comment

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

Oops trusted copilot's suggestions too much

/// Pros:
/// * Much better performance by rendering at lower resolution
/// * High quality temporal anti-aliasing
/// * Works on AMD, NVIDIA, and Intel GPUs
Copy link
Contributor

Choose a reason for hiding this comment

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

Web support?

Copy link
Author

Choose a reason for hiding this comment

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

Yes but future, I'll revise this

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what about macOS / android / iOS?

/// * Much better performance by rendering at lower resolution
/// * High quality temporal anti-aliasing
/// * Works on AMD, NVIDIA, and Intel GPUs
/// * Includes optional sharpening pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the optional sharpening pass make sense to include? We already have RCAS built into Bevy.

I'd rather people use that, as it's compatible with TAA/DLSS, and will play better once we make a generic TSR abstraction.

Copy link
Author

Choose a reason for hiding this comment

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

I haven't actually wired it up yet in FSR, but it reuses an existing pass, so I would expect it to be faster.

MainPassResolutionOverride(render_resolution),
));
} else {
// Update resolution override in case it changed
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't sat down to think about it, but why is this structured differently than the DLSS prepare code?

Copy link
Author

Choose a reason for hiding this comment

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

I was modeling after TAA more than DLSS, I'll have a better look how this can be made more consistent

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I would look at the DLSS code and copy that, it's much more 1:1 with how FSR should work.

.add_systems(
Render,
(
prepare_fsr3_jitter_and_context.in_set(RenderSystems::ManageViews),
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs .before(prepare_view_targets), since you modify the main camera usages.

mut commands: Commands,
mut texture_cache: ResMut<TextureCache>,
render_device: Res<RenderDevice>,
views: Query<(Entity, &ExtractedCamera, &ExtractedView, &Fsr3), With<Fsr3>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need With if you already have &Fsr3 in your query.

render_device: Res<RenderDevice>,
views: Query<(Entity, &ExtractedCamera, &ExtractedView, &Fsr3), With<Fsr3>>,
) {
for (entity, camera, view, fsr3) in &views {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow the newer pattern for texture-prepare systems as used in https://github.com/bevyengine/bevy/blob/main/crates/bevy_solari/src/realtime/prepare.rs (you can just store TextureView now as textures can be retrieved from views, I should update the solari code)

///
/// See [`Fsr3`] for more details.
#[derive(Default)]
pub struct Fsr3Plugin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please split this file up to match the DLSS file structure (mode, extract, prepare, node).

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.

Support for AMD FSR 2.0

3 participants