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

Cold specialization #16420

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Cold specialization #16420

wants to merge 13 commits into from

Conversation

tychedelia
Copy link
Contributor

Objective

  • Describe the objective or issue this PR addresses.
  • If you're fixing a specific issue, say "Fixes #X".

Solution

  • Describe the solution used to achieve the objective above.

Testing

  • Did you test these changes? If so, how?
  • Are there any parts that need more testing?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Showcase

This section is optional. If this PR does not include a visual change or does not add a new feature, you can delete this section.

  • Help others understand the result of this PR by showcasing your awesome work!
  • If this PR adds a new feature or public API, consider adding a brief pseudo-code snippet of it in action
  • If this PR includes a visual change, consider adding a screenshot, GIF, or video
    • If you want, you could even include a before/after comparison!
  • If the Migration Guide adequately covers the changes, you can delete this section

While a showcase should aim to be brief and digestible, you can use a toggleable section to save space on longer showcases:

Click to view showcase 
println!("My super cool code.");

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

  • If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
  • Simply adding new functionality is not a breaking change.
  • Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.

@tychedelia tychedelia added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Investigation This issue requires detective work to figure out what's going wrong labels Nov 17, 2024
@tychedelia tychedelia mentioned this pull request Dec 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 17, 2024
@tychedelia @nicopap
@pcwalton @alice-i-cecile @chescock
Add AssetChanged query filter (#16810)
df14443 
# Objective

Implement a new `AssetChanged` query filter that allows users to query
for entities whose related assets may have changed.

- Closes #5069
- Unblocks #16420. Currently, `cold-specialization`, a key rendering
optimization for unlocking ancillary benefits of the retained render
world, is blocked on being unable detect all scenarios in which an
entity's mesh/material changes using events and observers. An
`AssetChanged` filter will drastically simplify our implementation and
be more robust to future changes.

Originally implemented by @nicopap in #5080.

## Solution

- Adds a new `AssetChanged` query filter that initializes a
`AssetChanges<A>` resource that tracks changed assets and ticks in
`asset_events`.
- ~Reverts #13343 and changes the api of `get_state` to accept `impl
Into<UnsafeWorldCell<'w>>` to allow accessing the `AssetChanges<A>`
resource.~
- Adds a `AsAssetId` trait used for newtype handle wrappers (e.g.
`Mesh3d`) that allows associating a component with the underlying
`Asset` it represents.

## Testing

- Tests are added for `AssetChanged`.
- TBD on performance. We are going to add this `Mesh3d` and
`MeshMaterial3d` (etc) in the renderer. Long term wins in render
performance this unblocks should swamp tracking overhead for any
realistic workload.

## Migration Guide

- The `asset_events` system is no longer public. Users should order
their systems relative to the `AssetEvents` system set.

---------

Co-authored-by: Nicola Papale <nico@nicopap.ch>
Co-authored-by: Patrick Walton <pcwalton@mimiga.net>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@tychedelia @nicopap
@pcwalton @alice-i-cecile @chescock @ecoskey
Add AssetChanged query filter (bevyengine#16810)
53b44ec 
# Objective

Implement a new `AssetChanged` query filter that allows users to query
for entities whose related assets may have changed.

- Closes bevyengine#5069
- Unblocks bevyengine#16420. Currently, `cold-specialization`, a key rendering
optimization for unlocking ancillary benefits of the retained render
world, is blocked on being unable detect all scenarios in which an
entity's mesh/material changes using events and observers. An
`AssetChanged` filter will drastically simplify our implementation and
be more robust to future changes.

Originally implemented by @nicopap in bevyengine#5080.

## Solution

- Adds a new `AssetChanged` query filter that initializes a
`AssetChanges<A>` resource that tracks changed assets and ticks in
`asset_events`.
- ~Reverts bevyengine#13343 and changes the api of `get_state` to accept `impl
Into<UnsafeWorldCell<'w>>` to allow accessing the `AssetChanges<A>`
resource.~
- Adds a `AsAssetId` trait used for newtype handle wrappers (e.g.
`Mesh3d`) that allows associating a component with the underlying
`Asset` it represents.

## Testing

- Tests are added for `AssetChanged`.
- TBD on performance. We are going to add this `Mesh3d` and
`MeshMaterial3d` (etc) in the renderer. Long term wins in render
performance this unblocks should swamp tracking overhead for any
realistic workload.

## Migration Guide

- The `asset_events` system is no longer public. Users should order
their systems relative to the `AssetEvents` system set.

---------

Co-authored-by: Nicola Papale <nico@nicopap.ch>
Co-authored-by: Patrick Walton <pcwalton@mimiga.net>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Investigation This issue requires detective work to figure out what's going wrong
Projects
Rendering
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@tychedelia