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.

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

Split of referrence batches into Core/non-Core #628

Merged
merged 6 commits into from Mar 5, 2015

Conversation

@kvark
Copy link
Member

commented Mar 4, 2015

(This is a breaking change, please review carefully)

I noticed a couple of things that weren't sound:

  1. RefBatch carried a publicly accessible slice, while not carrying the parameters. The reason for the former was "convenience" (IIRC), the reason for the latter was "just because we can". Hence, this type was neither complete nor minimal.
  2. A lot of applications tend to keep a parameter structure together with a RefBatch. They are often seen together, and that wasn't convenient to do.

As a solution, I made a RefCore batch that only carries minimal (for safety) sealed data, nothing is available to public. The RefBatch now is rather open - it carries the core as well as a slice (like it used to) and a parameter structure, all mutable by public.

The idea is - RefCore to be used for maximum flexibility, like creating a batch when not yet having a parameter structure at hands, or using it with multiple parameters/slices. While RefBatch is just a convenient thing to have. The maintenance cost is minimal, since they are re-using lots of code.

RefBatch is also nice for ergonomic reasons - you can create a batch with type inference.

kvark added some commits Mar 4, 2015

@bvssvni

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2015

Looks good.

type Texture = ();
type Sampler = ();
}

This comment has been minimized.

Copy link
@brendanzab

brendanzab Mar 4, 2015

Member

Good stuff. Maybe impl on an enum TestResources {}?

This comment has been minimized.

Copy link
@kvark

kvark Mar 4, 2015

Author Member

Agreed

&mut self.context
}
}

This comment has been minimized.

Copy link
@brendanzab

brendanzab Mar 4, 2015

Member

Does this need to be in this PR?

This comment has been minimized.

Copy link
@kvark

kvark Mar 4, 2015

Author Member

This allows make_batch, make_core, and bind to be called from Graphics, instead of wrapping them up into pass-through methods.

This comment has been minimized.

Copy link
@brendanzab

brendanzab Mar 4, 2015

Member

Hmm, ok - just think it might be an abuse of Deref, which is a debate I would rather not have on this PR.

This comment has been minimized.

Copy link
@kvark

kvark Mar 4, 2015

Author Member

Feel free to open this debate anywhere please. I'm not too experienced with Deref practices and would be happy to know more.

@@ -236,66 +237,113 @@ impl<T: Clone + PartialEq> Array<T> {
}


/// Ref batch - copyable and smaller, but depends on the `Context`.
/// Referrenced core - a minimal sealed batch that depends on `Context`.

This comment has been minimized.

Copy link
@brendanzab

brendanzab Mar 4, 2015

Member

Referenced

This comment has been minimized.

Copy link
@kvark

kvark Mar 4, 2015

Author Member

will fix

/// It has references to the resources (mesh, program, state), that are held
/// by the context that created the batch, so these have to be used together.
pub struct RefBatch<T: ShaderParam> {
pub struct RefCore<T: ShaderParam> {

This comment has been minimized.

Copy link
@brendanzab

brendanzab Mar 4, 2015

Member

What other names have you thought of? What about RefBatchData?

This comment has been minimized.

Copy link
@kvark

kvark Mar 4, 2015

Author Member

I haven't really thought about names, no. RefBatchData doesn't sound too good because it bears the "data" term, while in fact RefCore can be seen as a batch type. Yes it is incomplete (requires not only the context, but also a slice and a parameter data), but neither is RefBatch (needs the context).

I don't insist on RefCore name though. My understanding is that since we don't re-export everything in batch module, this type is accessed as gfx::batch::RefCore, which makes it less ambiguous. Perhaps, you'd prefer CoreBatch more?

This comment has been minimized.

Copy link
@brendanzab

brendanzab Mar 4, 2015

Member

We can iterate on naming later, I think.

@kvark

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2015

One of the reasons behind the PR is also an attempt to standardize handle operations. I don't like the fact that some handles are copyable (BufferHandle) and others are only clonable (ProgramHandle). I think we should be consistent with this, and Clone is the common denominator. That's why Slice is no longer copyable, for instance.

@kvark

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2015

@stjahns this PR would improve this code in skeletal_animation:

pub struct SkinnedRenderer {
    animation_clip: AnimationClip,
    skinning_transforms_buffer: BufferHandle<GlResources, [[f32; 4]; 4]>,
    shader_params: SkinnedShaderParams<GlResources>,
    batch: RefBatch<SkinnedShaderParams<GlResources>>,
}

Also, I believe you really need this to be device-agnostic:

pub struct SkinnedRenderer<R: Resources> {
    animation_clip: AnimationClip,
    skinning_transforms_buffer: BufferHandle<R, [[f32; 4]; 4]>,
    batch: RefBatch<SkinnedShaderParams<R>>,
}

stjahns added a commit to stjahns/gfx-debug-draw that referenced this pull request Mar 4, 2015

Refactored to be backend-agnostic.
No assumption of OpenGL, except for lack of other shaders..
Depends on gfx-rs/gfx#628
@stjahns

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2015

Works for me / Am I doin' it rite?

@kvark kvark force-pushed the kvark:core branch from 03e1364 to 67c4dff Mar 5, 2015

@kvark

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2015

Self-merging as everything is fixed and the concept has been approved.

kvark added a commit that referenced this pull request Mar 5, 2015

Merge pull request #628 from kvark/core
Split of referrence batches into Core/non-Core

@kvark kvark merged commit 48e7a53 into gfx-rs:master Mar 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kvark kvark deleted the kvark:core branch Mar 5, 2015

@kvark kvark removed the status: working label Mar 5, 2015

@kvark kvark referenced this pull request Mar 5, 2015

Closed

Safe and convenient resource destruction #288

3 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.