bevyengine / bevyengine/bevy

Refactor `SubCameraView`

Open
#20,399 9 comments 0 reactions 0 assignees View on GitHub
A-Rendering C-Bug C-Usability D-Complex S-Ready-For-Implementation
Dominant language
Rust
Stars
48.2k
Forks
4.8k
Avg merge
3d 22h
Merged PRs (30d)
161

Description

Supercedes #20251

## Context

#15537 introduced the `SubCameraView` API, by copying the equivalent API from three.js. This resulted in a subpar implementation, however, with confusing documentation and undocumented panic conditions, amongst other things.

The only public API for this feature is the `sub_camera_view` field on `Camera`, which is an `Option`. The `SubCameraView` type is defined as:
```rust
pub struct SubCameraView {
pub full_size: UVec2,
pub offset: Vec2,
pub size: UVec2,
}
```
If set, this is used to calculate a new, smaller frustum for the camera, which is used for rendering instead of the camera's full frustum.

## The Problems

- ~~This smaller frustum must be entirely contained within the base, unmodified frustum. If it isn't, the current impl throws a WGPU validation error, which in turn throws a panic. As the API is just a few `pub` fields, with no helper methods, there are no safeguards to prevent this from happening.~~ This is caused by the camera's viewport going outside of the window, not directly related to `SubCameraView`.
- The size of the new frustum is defined as a fraction of the size of the base frustum. Instead of specifying this fraction directly, it is defined implicitly by the component-wise ratio of the values of the `size` and `full_size` fields. This is unnecessarily overcomplicated and confusing.
- The documentation is lackluster and fails both to explain how to use this feature, and to justify when to use it. The above panic conditions are also not mentioned anywhere.
- There is an example that showcases `SubCameraView`, but it is unhelpful and hard to understand.

## The Refactor

- The `size` and `full_size` fields should be folded into a single field that directly describes the scaling factor.
- They should be `Vec2` instead of `UVec2`, because they are already converted directly to `f32` everywhere they are used.
- ~~Alternatively, instead of scaling being done component-wise with a single `Vec2`, have two `f32` fields, one for each axis.~~ Actually, there should only be a single `f32` scaling value. Using a different scaling factor for each axis can change the aspect ratio of the subview to be different than that of the full frustum/viewport (which have their aspect ratios automatically kept in sync), which just causes the image to become distorted. I don't see a reason to want to do this intentionally.
- ~~None of the fields should be `pub`, instead there should be methods that enforce the invariants needed to not panic.~~ `SubCameraView` doesn't actually cause panics, although helper methods may still be useful.
- The documentation should explain how it actually works, including the units used, and examples of motivating use cases.
- The example should be simplified and reworked to clearly demonstrate the effects that different `SubCameraView` values have, compared to a camera that doesn't use the feature.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.