Associated resource type preparations #564

brendanzab · 2015-02-14T09:20:29Z

This should get us closer to implementing #416.

Currently referring to the Resources type via Device is quite ugly, eg. <<D as Device>::Resources as Resources>::Buffer, but apparently D::Resources::Buffer is going to be implemented in the future, which should make things nicer.

brendanzab · 2015-02-16T22:31:15Z

I think this is ready for review. Could you bump the cgmath version @kvark? I can't do it right now - I'm at work.

kvark · 2015-02-17T00:12:42Z

@bjz cgmath is published. I'll have a good look at the PR tomorrow.

brendanzab · 2015-02-17T03:31:00Z

Ok, it builds fine - needs a rebase though.

kvark · 2015-02-17T03:32:19Z

Did I just break your auto-merge?..

brendanzab · 2015-02-17T03:34:38Z

Yeah - that's ok. I can do it when I get home in several hours, or you are welcome to rebase for me.

This is a little ugly, but it allows us to have access to the resource types without CommandBuffer needing to have access to Device, which would result in a rustc stack overflow.

For now we just use `GlDevice` when we refer to them

kvark · 2015-02-17T16:51:41Z

Yay for Resources! I'm reviewing the whole thing now.

kvark · 2015-02-17T16:53:25Z

examples/deferred/main.rs

@@ -307,7 +307,9 @@ fn calculate_color(height: f32) -> [f32; 3] {
    }
 }

-fn create_g_buffer(width: u16, height: u16, device: &mut gfx::GlDevice) -> (gfx::Frame, TextureHandle, TextureHandle, TextureHandle, TextureHandle) {
+fn create_g_buffer(width: u16, height: u16, device: &mut gfx::GlDevice)
+        -> (gfx::Frame, TextureHandle<gfx::GlResources>, TextureHandle<gfx::GlResources>,


Since the user has to access gfx_device_gl crate anyway in order to create he device, perhaps we could provide a typedef for TextureHandle in there?

kvark · 2015-02-17T17:10:16Z

src/device/lib.rs

@@ -368,56 +364,56 @@ pub trait Device {
    fn submit(&mut self, buffer: (&Self::CommandBuffer, &draw::DataBuffer));

    // resource creation
-    fn create_buffer_raw(&mut self, size: usize, usage: BufferUsage) -> BufferHandle<()>;
-    fn create_buffer<T>(&mut self, num: usize, usage: BufferUsage) -> BufferHandle<T> {
+    fn create_buffer_raw(&mut self, size: usize, usage: BufferUsage) -> BufferHandle<back::GlResources, ()>;


Wait, this looks wrong. Shouldn't this be BufferHandle<Self::Resources, ()> instead?

I am just hard-coding it to GlDevice for now. That way we don't have to tackle everything at once.

kvark · 2015-02-17T17:14:46Z

@bjz Ok, I finished the first pass, waiting for you to comment. I assume you still had some technical issues preventing us to go fully generic on the device. Please describe them in detail.

Also, it would be great if @csherratt had a look here as well. The PR is big and important.

brendanzab · 2015-02-17T19:54:48Z

The reasons for not going fully generic on the device yet was mainly just to keep the PR under control. It is going to be a big effort to convert the Render crate as well.

kvark · 2015-02-17T19:58:47Z

Sure thing. I'd be happy to finish the conversion, as well as move the device out of gfx_device_gl.

Let me know when you are ready for the merge. Also - are you confident enough to proceed without @csherratt review?

ghost · 2015-02-18T03:37:03Z

Look ok to me.

kvark · 2015-02-18T03:40:35Z

@csherratt Thanks for having a look! Nice catch about Send though. If it doesn't work, that would prevent my apps (Asteroids, at least) from running. @bjz ?

brendanzab · 2015-02-18T04:20:40Z

Maybe we should add that as a constraint for the resources?

brendanzab · 2015-02-18T04:21:10Z

It wasn't me that picked it up - it was the type checker ^_^

kvark · 2015-02-18T04:26:11Z

@bjz please add the constraint to the resources, if you can.
Also, perhaps bring back the static reset command buffer.

brendanzab · 2015-02-18T04:31:42Z

Yeah, sure thing - will do it when I get home.

kvark · 2015-02-18T04:35:17Z

@bjz when is this gonna happen, roughly?

brendanzab · 2015-02-18T10:14:20Z

Done!

brendanzab · 2015-02-18T10:15:54Z

This is ready to merge by the way, whenever you are free. I'm now working on decoupling the renderer.

kvark · 2015-02-18T12:59:25Z

@bjz Awesome, thanks!

Associated resource type preparations

ghost · 2015-02-19T10:30:38Z

Sorry to ask this, but did anyone get this change working? I walked back to try and figure out what the root cause of a regression was and found that everything works as expected up until. brendanzab@c25ef1d

None of the examples are working after this change, Even the trivial triangle one. Using api trace

glDrawArrays(GL_TRIANGLES, 0, 3) --> glDrawArrays(GL_POINTS, 0, 0)

So something really screwy is going on here. I'm using rustc 1.0.0-nightly (b63cee4a1 2015-02-14 17:01:11 +0000)

brendanzab · 2015-02-19T14:24:11Z

Ack. Sorry! This is quite strange - can't see any transcription error in the diff. If you need to roll back on master feel free - I don't have much time to work on it tonight.

brendanzab · 2015-02-19T14:24:56Z

The triangle works for one frame, then disappears. :/

bvssvni · 2015-02-19T14:40:41Z

Macros seem to be broken with rustc 1.0.0-nightly (6c065fc8c 2015-02-17) (built 2015-02-18).

kvark · 2015-02-19T15:18:53Z

@bvssvni didn't I fix them in #576 ?

Apparently the new changes hit the same rustc bug that was plaguing hematite.

kvark · 2015-02-19T16:11:28Z

@bjz I wish you could show up a bit more often... Is gitter not allowed at your workplace?
Are you still working on decoupling the device?

kvark added the status: working label Feb 14, 2015

brendanzab changed the title ~~Associated type preparations~~ Associated resource type preparations Feb 14, 2015

brendanzab force-pushed the associated-type-prep branch 6 times, most recently from 46e9ba8 to 0c83286 Compare February 15, 2015 03:57

brendanzab mentioned this pull request Feb 15, 2015

Lifetime error with associated types, methods, and type aliases. rust-lang/rust#22356

Closed

brendanzab added 10 commits February 17, 2015 17:19

Parameterise Renderer over Device

84037df

Rename CommandBufferHelper->CommandBufferExt

326bd37

Move Command into gl module

7d4c9dd

Don't re-export opengl command buffer

6bd9c2d

Add associated resource types to Device

01f212d

Add resource types to CommandBuffer

6337291

This is a little ugly, but it allows us to have access to the resource types without CommandBuffer needing to have access to Device, which would result in a rustc stack overflow.

Parameterise handles over Device

c25ef1d

For now we just use `GlDevice` when we refer to them

Attempt to use a generic Device for BufferHandle impl

b9ca884

Add temporary fix for rust-lang/rust#22356

28f69e1

Add Resources trait for common resource types

af64f3e

brendanzab force-pushed the associated-type-prep branch from 1ee52a8 to af64f3e Compare February 17, 2015 06:19

kvark reviewed Feb 17, 2015
View reviewed changes

This was referenced Feb 18, 2015

[WIP] Use associated types for device resources #562

Closed

[WIP] Moving Device to associated types #517

Closed

Add Sync and Send to resource types

349ad7b

kvark added a commit that referenced this pull request Feb 18, 2015

Merge pull request #564 from bjz/associated-type-prep

dc200c6

Associated resource type preparations

kvark merged commit dc200c6 into gfx-rs:master Feb 18, 2015

kvark removed the status: working label Feb 18, 2015

brendanzab deleted the associated-type-prep branch February 18, 2015 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Associated resource type preparations #564

Associated resource type preparations #564

brendanzab commented Feb 14, 2015

brendanzab commented Feb 16, 2015

kvark commented Feb 17, 2015

brendanzab commented Feb 17, 2015

kvark commented Feb 17, 2015

brendanzab commented Feb 17, 2015

kvark commented Feb 17, 2015

kvark Feb 17, 2015

kvark Feb 17, 2015

brendanzab Feb 17, 2015

kvark commented Feb 17, 2015

brendanzab commented Feb 17, 2015

kvark commented Feb 17, 2015

ghost commented Feb 18, 2015

kvark commented Feb 18, 2015

brendanzab commented Feb 18, 2015

brendanzab commented Feb 18, 2015

kvark commented Feb 18, 2015

brendanzab commented Feb 18, 2015

kvark commented Feb 18, 2015

brendanzab commented Feb 18, 2015

brendanzab commented Feb 18, 2015

kvark commented Feb 18, 2015

ghost commented Feb 19, 2015

brendanzab commented Feb 19, 2015

brendanzab commented Feb 19, 2015

bvssvni commented Feb 19, 2015

kvark commented Feb 19, 2015

kvark commented Feb 19, 2015

Associated resource type preparations #564

Associated resource type preparations #564

Conversation

brendanzab commented Feb 14, 2015

brendanzab commented Feb 16, 2015

kvark commented Feb 17, 2015

brendanzab commented Feb 17, 2015

kvark commented Feb 17, 2015

brendanzab commented Feb 17, 2015

kvark commented Feb 17, 2015

kvark Feb 17, 2015

Choose a reason for hiding this comment

kvark Feb 17, 2015

Choose a reason for hiding this comment

brendanzab Feb 17, 2015

Choose a reason for hiding this comment

kvark commented Feb 17, 2015

brendanzab commented Feb 17, 2015

kvark commented Feb 17, 2015

ghost commented Feb 18, 2015

kvark commented Feb 18, 2015

brendanzab commented Feb 18, 2015

brendanzab commented Feb 18, 2015

kvark commented Feb 18, 2015

brendanzab commented Feb 18, 2015

kvark commented Feb 18, 2015

brendanzab commented Feb 18, 2015

brendanzab commented Feb 18, 2015

kvark commented Feb 18, 2015

ghost commented Feb 19, 2015

brendanzab commented Feb 19, 2015

brendanzab commented Feb 19, 2015

bvssvni commented Feb 19, 2015

kvark commented Feb 19, 2015

kvark commented Feb 19, 2015