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.

Sign up for GitHub

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

Jump to bottom

Mix of HDR and non-HDR cameras breaks rendering #6754

Closed
djeedai opened this issue Nov 25, 2022 · 7 comments · Fixed by #13419
Closed

Mix of HDR and non-HDR cameras breaks rendering #6754

djeedai opened this issue Nov 25, 2022 · 7 comments · Fixed by #13419
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Comments

@djeedai
Copy link
Contributor

djeedai commented Nov 25, 2022

Bevy version

0.9 and main

Relevant system information

AdapterInfo { name: "NVIDIA GeForce RTX 2070", vendor: 4318, device: 7938, device_type: DiscreteGpu, driver: "NVIDIA", driver_info: "517.48", backend: Vulkan }

What you did

Change the split_screen.rs example to add hdr: true on one camera but not the other (doesn't matter which one).

What went wrong

Background becomes black, default clear color (grey) not applied anymore. I both cases (whether hdr is set on left or right), only the right camera shows something.

image

Testing this on another 4-camera 🎆 Hanabi example, where things animate, I can confirm clearing doesn't work (all particles render on the previous frame) and the HDR camera stops rendering:

image

Although the latter could be a bug in 🎆 Hanabi, the similarity of artifacts might help diagnosing the issue.

Additional information

I'm guessing the cameras have incompatible render target, and there's no "smart" resolution applying tonemapping only to HDR ones.

It's unclear if this is supposed to work in the first place but logged as bug because @superdump said it should "maybe" be supported.
@djeedai djeedai added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Investigation This issue requires detective work to figure out what's going wrong labels Nov 25, 2022
@superdump
Copy link
Contributor

There are two problems:

  • The upscaling render pass unconditionally clears the RenderPassColorAttachment
  • The UpscalingPipeline RenderPipelineDescriptor configures None for the colour attachment's blend state

If I instead unconditionally load the colour attachment texture, and alpha blend it, then the desired result is produced. I think the following should be done as a proper solution:

  • Clear the out texture only for the initial upscale into it, otherwise load it (or make this configurable)
  • Use alpha blending for composing one camera on top of another - not sure whether other options make more sense?

@torsteingrindvik
Copy link
Contributor

Think I found the same issue.

I'll copy paste my question from Discord:

image

@JMS55 JMS55 added this to the 0.13 milestone Oct 27, 2023
@JMS55
Copy link
Contributor

JMS55 commented Jan 8, 2024

#10325 should let us implement a proper fix for this now

@JMS55
Copy link
Contributor

JMS55 commented Jan 26, 2024

We don't need clear_color: ClearColorConfig::None in the split screen example anymore.

@JMS55 JMS55 modified the milestones: 0.13, 0.14 Feb 1, 2024
@migimunz
Copy link

migimunz commented Apr 3, 2024

Also happening on 0.13.1. Here's a minimal example that replicates it.

Tested on M1: AdapterInfo { name: "Apple M1 Max", vendor: 0, device: 0, device_type: IntegratedGpu, driver: "", driver_info: "", backend: Metal }

@alice-i-cecile alice-i-cecile added S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! and removed S-Needs-Investigation This issue requires detective work to figure out what's going wrong labels May 16, 2024
@tychedelia
Copy link
Contributor

I'd like to tackle this tomorrow if no one is already working on it.

@JMS55
Copy link
Contributor

JMS55 commented May 17, 2024

No one else is working on it, feel free to.

tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
55012f1 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
999e3ac 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
509c075 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
12a4610 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
b54ca20 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
407fc1a 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
8edc302 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
tychedelia added a commit to tychedelia/bevy that referenced this issue May 18, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target
5e80886 
Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
@tychedelia tychedelia mentioned this issue May 18, 2024
Merged
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels May 26, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.14 milestone Jun 4, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 6, 2024
@tychedelia
Allow mix of hdr and non-hdr cameras to same render target (#13419)
027f8e2 
Changes:
- Track whether an output texture has been written to yet and only clear
it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing the
upscaling pipeline and use alpha blending for extra cameras rendering to
that texture that do not specify an explicit blend mode.

Fixes #6754

## Testing

Tested against provided test case in issue:

![image](https://github.com/bevyengine/bevy/assets/10366310/d066f069-87fb-4249-a4d9-b6cb1751971b)

---

## Changelog

- Allow cameras rendering to the same output texture with mixed hdr to
work correctly.

## Migration Guide

- - Change `CameraOutputMode` to use `ClearColorConfig` instead of
`LoadOp`.
mockersf pushed a commit that referenced this issue Jun 6, 2024
@tychedelia @mockersf
Allow mix of hdr and non-hdr cameras to same render target (#13419)
4320444 
Changes:
- Track whether an output texture has been written to yet and only clear
it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing the
upscaling pipeline and use alpha blending for extra cameras rendering to
that texture that do not specify an explicit blend mode.

Fixes #6754

## Testing

Tested against provided test case in issue:

![image](https://github.com/bevyengine/bevy/assets/10366310/d066f069-87fb-4249-a4d9-b6cb1751971b)

---

## Changelog

- Allow cameras rendering to the same output texture with mixed hdr to
work correctly.

## Migration Guide

- - Change `CameraOutputMode` to use `ClearColorConfig` instead of
`LoadOp`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow mix of hdr and non-hdr cameras to same render target tychedelia/bevy
7 participants
@superdump @migimunz @alice-i-cecile @djeedai @tychedelia @JMS55 @torsteingrindvik