Skip to content

Rendertarget cmp (adopted)#15353

Closed
bas-ie wants to merge 3 commits intobevyengine:mainfrom
bas-ie:rendertarget-cmp
Closed

Rendertarget cmp (adopted)#15353
bas-ie wants to merge 3 commits intobevyengine:mainfrom
bas-ie:rendertarget-cmp

Conversation

@bas-ie
Copy link
Copy Markdown
Contributor

@bas-ie bas-ie commented Sep 22, 2024 •
edited
Loading

This is an adoption of #8300 (aevyrie). The only change I've made is removing Ord as suggested, and PartialOrd because I presumed that made sense.

Also took out FromReflect, because I see we get that for free now with Reflect.

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.

@bas-ie bas-ie mentioned this pull request Sep 22, 2024
Closed
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 22, 2024
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 22, 2024
@bas-ie
Copy link
Copy Markdown
Contributor Author

bas-ie commented Sep 22, 2024

Ah, yes I was possible a bit naiive here! Given the CI failure, I presume I would need to conduct some surgery on sort_cameras to make this work?

@tychedelia
Copy link
Copy Markdown
Member

tychedelia commented Sep 22, 2024 •
edited
Loading

Ah, yes I was possible a bit naiive here! Given the CI failure, I presume I would need to conduct some surgery on sort_cameras to make this work?

I'm not sure I totally understand the original justification for having different render targets in the same order. The implicit sort just feels like a bug waiting to happen. I think we should change the ambiguity code to enforce an explicit total order (i.e. unique per camera). Curious what others think.

@bas-ie
Copy link
Copy Markdown
Contributor Author

bas-ie commented Sep 22, 2024 •
edited
Loading

I'm not sure I totally understand the original justification for having different render targets in the same order. The implicit sort just feels like a bug waiting to happen. I think we should change the ambiguity code to enforce an explicit total order (i.e. unique per camera). Curious what others think.

Awesome, I definitely am not qualified for that so I'll wait to hear the result!

@bas-ie bas-ie deleted the rendertarget-cmp branch September 23, 2024 22:05
@bas-ie
Copy link
Copy Markdown
Contributor Author

bas-ie commented Sep 23, 2024

Lack of clear consensus here in 2024, Discord discussion for further context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tychedelia tychedelia Awaiting requested review from tychedelia

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

None yet

Milestone

0.15

Development

Successfully merging this pull request may close these issues.

4 participants

@bas-ie @tychedelia @alice-i-cecile @aevyrie