Allow RenderTarget comparison

[aevyrie]

Objective

• The changes in Windows as Entities removed the ability to compare RenderTargets without bringing in additional query data. This should not be needed, as these types are directly comparable.

Solution

• Reinstate derive macros.

[james7132]

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?

[aevyrie]

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.

[james7132]

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.

[jakobhellermann]

This will return false for RenderTarget::Window(WindowRef::Primary) == RenderTarget::Window(WindowRef::Entity(primary_window)), right? Is that a problem?

[aevyrie]

I kinda think that's a problem with the new RenderTarget changes. I've found the recent changes to the type difficult to work with, and the special casing for primary windows makes it awkward to use. The fact that you can represent identical states with different values is a code smell to me, and not something I think we should patch over with more code.

Anywho, the larger point here is that I should be able to take two RenderTargets and check if they are equal (without needing to do additional ECS queries!). I'm fine with axing Ord and friends, the only reason I included those was to mirror the sister Normalized type.

[cart]

I'm on board for these changes, although removing Ord makes sense if we don't have a reasonable use case for it, especially given the conversation above.

I think I'm even on board for WindowRef::Primary != WindowRef::Entity(PRIMARY_WINDOW_ENTITY), provided we document that.

I kinda think that's a problem with the new RenderTarget changes. I've found the recent changes to the type difficult to work with, and the special casing for primary windows makes it awkward to use. The fact that you can represent identical states with different values is a code smell to me, and not something I think we should patch over with more code.

The core issue is that you can't have a statically defined "default entity id" (like we could for WindowId::primary because we owned the entire id space for WindowId). If we remove WindowRef::Primary, that would mean that RenderTarget could no longer have a default window value, which means Cameras could no longer have a default value.

There are a variety of alternatives, I just think they're all worse than the current approach:

1. Re-add WindowId and make it a component. For RenderTargets and other window references, use that instead of Entities. This feels weird to me (each window has two canonical ids) and makes a number of assignment scenarios harder than entities (ex: spawn a window with a command, grab the newly created WindowId, assign to camera render target).
2. Use RenderTarget::Window(Option<Entity>) and "fix up" the None value on the first frame with the primary window. I've been pushing against the "fix up" pattern in Bevy for awhile. As much as possible I don't want to be touching people's entities. If a user sets a value on a component, it should stay that way to avoid surprises. This pattern also produces change events that users don't expect. And imo as much as possible entities should be in a "correct" state on insert. They shouldn't be "made correct' at some arbitrary point in the schedule. This also doesn't really solve the comparison problem because None != Some(PRIMARY_WINDOW_ENTITY) is essentially the same thing.

[cart]

I suppose theres also option (3): don't impl Default for Camera and require people to assign RenderTarget values (and everything else) themselves. But thats not actually an option for so many different reasons that I'm not going to list them unless someone really wants to advocate for this.

[aevyrie]

I'm in favor of removing Ord for for RenderTarget and NormalizedRenderTarget. I'm not sure that order makes much sense for these types, and users may have different ordering needs and expectations. Would it be acceptable to include that change in this PR?

[cart]

Yeah I'm on board for that

[bas-ie]

Adopted in #15353 .