bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Merge PipelineIds and CachedPipelineIds

Open kurtkuehnert opened this issue 2 years ago • 7 comments

Objective

  • Motivated by #7188
  • Simplifies the pipeline IDs.

Currently, we use atomic IDs for all of our render resources (RenderPipelineId, ComputePipelineId). Additionally, we assign a unique CachedRenderPipelineId/CachedComputePipelineId to each of our pipelines in the PipelineCache.

This PR merges both of them together, to simplify the API.

Solution

  • This PR uses the atomically incrementing IDs directly to index into a cached pipeline vector. This enables cheap and safe lookups.
  • This implies that all pipelines have to be created via the PipelineCache.
  • Simply queue your pipeline into the cache (or let a specialized pipeline do the job) and retrieve it via its ID.
  • We can still create raw pipelines using the wgpu API directly, to allow for third-party integration.

Changelog

  • replaced all occurrences of CachedRenderPipelineId and CachedComputePipelineId in favor of RenderPipelineId and ComputePipelineId
  • removed PipelineCache::pipelines
  • renamed PipelineCache::get_render_pipeline_state to PipelineCache::render_pipeline_state
  • renamed PipelineCache::get_compute_pipeline_state to PipelineCache::compute_pipeline_state
  • renamed PipelineCache::get_render_pipeline_descriptor to PipelineCache::render_pipeline_descriptor
  • renamed PipelineCache::get_compute_pipeline_descriptor to PipelineCache::compute_pipeline_descriptor

Migration Guide

  • replaced all occurrences of CachedRenderPipelineId and CachedComputePipelineId in favor of RenderPipelineId and ComputePipelineId
  • removed PipelineCache::pipelines
  • renamed PipelineCache::get_render_pipeline_state to PipelineCache::render_pipeline_state
  • renamed PipelineCache::get_compute_pipeline_state to PipelineCache::compute_pipeline_state
  • renamed PipelineCache::get_render_pipeline_descriptor to PipelineCache::render_pipeline_descriptor
  • renamed PipelineCache::get_compute_pipeline_descriptor to PipelineCache::compute_pipeline_descriptor

kurtkuehnert avatar Feb 01 '23 14:02 kurtkuehnert

My first thought when seeing the description of this PR is that when we start removing unused pipelines, how do we manage that they have been removed so we can destroy them? That makes me think of generation indices, and that makes me think "pipelines as entities?" :)

superdump avatar Feb 01 '23 18:02 superdump

Mhh, that sounds interesting. Not sure what that would look like though. This solution is not really a regression, because we have been doing this up until now anyways.

I will think about it.

kurtkuehnert avatar Feb 01 '23 18:02 kurtkuehnert

Also we would probably need persistent entities in the render world first.

kurtkuehnert avatar Feb 01 '23 18:02 kurtkuehnert

I have made a quick demo of the concept here.

I really like this design, unfortunately, we still can't persist entities in the render world.

fn old(pipeline_cache: Res<PipelineCache>) {
    let descriptor = RenderPipelineDescriptor::default();
    let id = pipeline_cache.queue_render_pipeline(descriptor);
    
    let pipeline = pipeline_cache.get_render_pipeline(id).unwrap();
}

fn new(mut commands: Commands, pipelines: Query<&RenderPipeline>) {
    let descriptor = RenderPipelineDescriptor::default();
    let entity = commands.spawn((descriptor, PipelineState::Queued));
    // or
    let entity = commands.add_render_pipeline(descriptor);
    
    let pipeline = pipelines.get(entity).unwrap();
}

But once that is resolved, I will implement this.

kurtkuehnert avatar Feb 01 '23 19:02 kurtkuehnert

Could we separate this from #7188? It's unclear if we want to merge those two, and I would like for this redundancy to be removed.

james7132 avatar Feb 03 '23 04:02 james7132

Yeah sure, I will do that today. 👍

kurtkuehnert avatar Feb 03 '23 07:02 kurtkuehnert

Could we separate this from #7188? It's unclear if we want to merge those two, and I would like for this redundancy to be removed.

This PR is now independent.

I still believe that there is value in both PRs. #7188 should not be controversial if we can not store raw wgpu Ids anyway. Also, it has already gotten cart's approval (and it's a pretty trivial change).

I'm on board for the move toward simplification / more opinionated apis and we can then break out when the need arises.

kurtkuehnert avatar Feb 03 '23 13:02 kurtkuehnert