Skip to content

Improve spec_v2 patterns #20114

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.

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Jul 13, 2025

Objective

  • Adds some util methods to remove some boilerplate from specializers. More will probably be added later but set_target and set_layout will be the most used I think.
    • Note: Specializers can't rely on their input descriptor having a certain shape, so instead of just pushing to each vec, the methods pad the length of the vec if necessary and set the value directly.
  • After migrating a few engine Specializers, GetBaseDescriptor & SpecializedCache: Resource both seem like anti-patterns, especially with dynamic materials on the horizon
  • Also removes user_specializers. If anyone needs that functionality they can easily make a wrapper for it.

Solution

  • Add the things
  • Nuke the stuff
  • update the migration guide

@ecoskey ecoskey added A-Rendering Drawing game state to the screen S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Jul 13, 2025
@ecoskey ecoskey added 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 and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Jul 13, 2025
@ecoskey ecoskey added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Jul 13, 2025
@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 14, 2025
@ecoskey ecoskey changed the title Nuke GetBaseDescriptor Improve specv2 patterns Jul 15, 2025
@ecoskey ecoskey changed the title Improve specv2 patterns Improve spec_v2 patterns Jul 15, 2025
@ecoskey ecoskey added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jul 15, 2025
@ecoskey
Copy link
Contributor Author

ecoskey commented Jul 15, 2025

moved the blocking items to this pr, so it should be merged before the others actually

@ecoskey ecoskey added this to the 0.17 milestone Jul 15, 2025
Copy link
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

This approach seems like a nice cleanup, just a few questions.

@@ -62,3 +64,19 @@ impl Deref for BindGroupLayout {
&self.value
}
}

static EMPTY_BIND_GROUP_LAYOUT: OnceLock<BindGroupLayout> = OnceLock::new();
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so we now use an empty layout in a few places post wgpu 25, but I'm a little wary about this. I guess the set_at api is nicer than just doing a push like we do in some places, but I'm not sure I understand why it should ever be necessary to implicitly fill in the holes in a layout?

Copy link
Contributor Author

@ecoskey ecoskey Jul 16, 2025

Choose a reason for hiding this comment

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

why it should ever be necessary to implicitly fill in the holes in a layout?

Only with the assumption that it'll be properly filled in by other logic, like by composing specializers.

There's probably a better solution than filling_set_at, but since specializers can't really rely on their input having a certain shape (like what the current length of the vec is) both push and direct indexing cause problems without bounds checks or padding.

pub struct SpecializedCache<T: Specializable, S: Specializer<T>> {
specializer: S,
user_specializer: Option<SpecializerFn<T, S>>,
Copy link
Member

Choose a reason for hiding this comment

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

What's the thought on how this should work now? Should we copy the specialization fn pointer into whatever specializer component we create?

Copy link
Contributor Author

@ecoskey ecoskey Jul 16, 2025

Choose a reason for hiding this comment

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

We could add a simple wrapper too:

struct WithUserSpecializer<T: Specializable, S: Specializer<T>> {
  specializer: S,
  user_specializer: SpecializerFn<T, S>
}

I just think it's a bit niche to put in the collection itself. Also (marginally) easier to iterate on if we want to turn user_specializer into a Box<dyn DynSpecialize> or something later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants