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

Allow RenderTarget comparison #8300

Closed
wants to merge 1 commit into from
Closed

Allow RenderTarget comparison #8300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_render/src/camera/camera.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ impl CameraRenderGraph {

/// The "target" that a [`Camera`] will render to. For example, this could be a [`Window`](bevy_window::Window)
/// swapchain or an [`Image`].
#[derive(Debug, Clone, Reflect)]
#[derive(Debug, Clone, Reflect, PartialEq, Eq, Hash, PartialOrd, Ord)]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if Ord can be implemented here properly. Are we potentially arbitrarily stating that all Window variants are less than Image variants? Does that even make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mirrored the derives from NormalizedRenderTarget to stay consistent. I assume these are derived for use cases where you want an ordered list or hashmap keys or similar.

Copy link
Member

Choose a reason for hiding this comment

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

Upon checking the docs for the stdlib, it occurred to me the exact ordering behavior of the derive macros are not well documented/defined. Filed rust-lang/rust#109946 as a follow-up.

If enum variants matter, and we want a first-to-last ordering for render targets, I'm tempted to say we should put Image variants before Window, since that tends to be how you'd expect it to be sorted in a rendering order, and we should probably document that too. This can be done in this PR or as a follow-up.

pub enum RenderTarget {
/// Window to which the camera's view is rendered.
Window(WindowRef),
Expand Down
4 changes: 3 additions & 1 deletion crates/bevy_window/src/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ pub struct PrimaryWindow;
/// Reference to a window, whether it be a direct link to a specific entity or
/// a more vague defaulting choice.
#[repr(C)]
#[derive(Default, Copy, Clone, Debug, Reflect, FromReflect)]
#[derive(
Default, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Reflect, FromReflect,
)]
#[cfg_attr(
feature = "serialize",
derive(serde::Serialize, serde::Deserialize),
Expand Down