What if Ash #12

zakarumych · 2018-10-28T20:33:09Z

This PR accumulates lots of commits I made for last weekend.
Overview:

Abandon hal for now.
Restructure into more self-contained crates. Main crate only reexports sub-crates
Add examples to main crate:
1. init example loads Vulkan by instantiating Factory.
2. window example uses wsi sub-crate to create render target for window.
3. simple example (WIP) draws simple mesh.

This change is

LucioFranco

This is awesome! Looking forward to using it. 👍

LucioFranco · 2018-10-28T21:09:39Z

factory/src/config.rs

+}
+
+#[allow(unused)]
+fn fmt_version(version: &u32, fmt: &mut ::std::fmt::Formatter) -> Result<(), ::std::fmt::Error> {


You can use std::fmt::Formatter now in 1.30 😄

Finally.
Can I omit extern crate <name>; now?

I don't think so that will probably come in the next few releases as we inch closer to 2018 edition

LucioFranco · 2018-10-28T22:02:31Z

factory/src/factory.rs

+    extensions: Vec<ExtensionProperties>,
+}
+
+/// The `Factory<D>` type represents the overall creation type for `rendy`.


You can remove the generic on the Factory type I believe

LucioFranco · 2018-10-28T22:31:26Z

mesh/README.md

@@ -0,0 +1,38 @@
+
+# `gfx-mesh`


LucioFranco · 2018-10-29T00:15:13Z

rendy/Cargo.toml

-hal = ["gfx-hal", "rendy-memory/hal", "rendy-resource/hal", "rendy-command/hal"]
-vulkan = ["ash", "rendy-memory/vulkan", "rendy-resource/vulkan", "rendy-command/vulkan"]
+[dev-dependencies]
+ash = { path = "../../ash/ash" }


Could you say what version of ash? I'm assuming off of the code that is in the WIP builder branch of ash?

It's MatusT's fork, generator branch, with my patch.
It must be in main repo generator branch soon (before merge).
If needed I can put it into my own fork for now.

Yeah, I think that would be useful so I can try and build this locally as well 👍

dotellie

Can I say wow? Awesome job Viral! I'm still not 100% convinced about removing hal considering we already had so much of the abstraction in place, but I'm sure you have a good plan for it.

dotellie · 2018-10-29T09:50:29Z

factory/src/config.rs

+    }
+}
+
+pub unsafe trait HeapsConfigure {


This name seems a bit off to me. Shouldn't it be something like HeapsConfig or HeapsConfiguration?

It's not a configuration, it's a configurer. I would opt for something like ConfigureHeaps or HeapsConfigurer.

dotellie · 2018-10-29T09:55:10Z

factory/src/factory.rs

+        }?;
+        trace!("Instance created");
+
+        let surface = Surface::new(&entry, &instance)


Should we really provide a surface by default? I may be wrong here, but in VR for example, although you do almost always end up with a window somehow, it might now be desirable to have. Rather you would just use whatever you get from the compositor.

No, we shouldn't. We'll add it to configuration.

dotellie · 2018-10-29T10:05:34Z

factory/src/factory.rs

+                            .application_name(&CString::new(config.app_name)?)
+                            .application_version(config.app_version)
+                            .build(),
+                    ).enabled_extension_names(&extensions_to_enable(&extensions)?)


Is this just enabling all extensions unconditionally? I think a better solution would be to have a set of required extensions that rendy always requires and then allow the user to pass in what extensions they need.

dotellie · 2018-10-29T10:07:05Z

factory/src/factory.rs

+                physical.handle,
+                &DeviceCreateInfo::builder()
+                    .queue_create_infos(&create_queues)
+                    .enabled_extension_names(&extensions_to_enable(&physical.extensions).unwrap())


We need a callback here to enable more extensions. OpenVR (and probably more XR compositors) require to be able to enable specific extensions based off of a physical device.

Definitely. I'll add a TODO

dotellie · 2018-10-29T10:08:56Z

factory/src/factory.rs

+            .cloned()
+            .filter_map(|name| {
+                let cstr_name = CStr::from_ptr(name);
+                trace!("Look for {:?}", cstr_name);


Can you clarify what you're looking for here so the logs aren't too confusing to someone that doesn't really know graphics programming? 😅

That's my debugging stuff 😅

dotellie · 2018-10-29T10:15:22Z

memory/src/allocator/arena.rs

-    #[derivative(Debug(bound = "T: Debug", format_with = "super::memory_ptr_fmt"))]
-    memory: *const Memory<T>,
+pub struct ArenaBlock {
+    // #[derivative(Debug(format_with = "::memory::memory_ptr_fmt"))]


Commented code

Derivative seems broken somehow. I'll uncomment once it works again.

dotellie · 2018-10-29T10:17:20Z

memory/src/allocator/dynamic.rs

    index: u32,
-    #[derivative(Debug(bound = "T: Debug", format_with = "super::memory_ptr_fmt"))]
-    memory: *const Memory<T>,
+    // #[derivative(Debug(format_with = "super::memory_ptr_fmt"))]


Commented code

dotellie · 2018-10-29T10:20:41Z

memory/src/heaps.rs

        match block {
            BlockFlavor::Dedicated(block) => self.dedicated.free(device, block),
            BlockFlavor::Arena(block) => self.arena.as_mut().unwrap().free(device, block),
            BlockFlavor::Dynamic(block) => self.dynamic.as_mut().unwrap().free(device, block),
-            // BlockFlavor::Chunk(block) => self.chunk.free(device, block),


How come you removed this line when you still have Chunk commented out on the various config structs?

Should remove it everywhere.

dotellie · 2018-10-29T10:24:23Z

mesh/src/mesh.rs

+                        len,
+                    })
+                }).collect::<Result<_, Error>>()?,
+            ibuf: match self.indices {


I think this match can be replaced with a simple .map()

dotellie · 2018-10-29T10:25:16Z

mesh/src/mesh.rs

+
+/// Error type returned by `Mesh::bind` in case of mesh's vertex buffers are incompatible with requested vertex formats.
+#[derive(Clone, Copy, Debug)]
+pub struct Incompatible;


Shouldn't we have a failure error enum here like for the other crates?

kvark · 2018-10-29T13:45:47Z

What's the reasoning behind "Abandon hal for now."?

Rhuagh · 2018-10-30T12:52:22Z

memory/src/allocator/arena.rs

-            "Arenas are not empty during allocator disposal. Arenas: {:#?}",
-            self.arenas
-        );
+        if !self.arenas.is_empty() {


Add debug_assert maybe? So it crashes on debug mode, but only logs on release?

zakarumych · 2018-10-30T15:06:52Z

@kvark without reinventing the wheel (abstraction layer over ash and hal 😓) I'm way more productive.

Instead of providing own abstractions I believe it is better to implement ash::DeviceV* etc traits for hal. I heard that someone already suggesting to do so (or even started).
Then to work over hal rendy would only need to parametrize Factory type.

Moxinilian · 2018-10-31T01:20:24Z

To the reader: please note that "dropping hal" does not mean that hal will not have anything to do with rendy. Viral is focusing on ash right now as it is closer to Vulkan so easier to use for him, but when all of this is done rendy is made in such a way that it will be "easy" to implement the hal backend. It's just to not have to implement two backends at once.

kvark · 2018-10-31T01:30:40Z

@Moxinilian thank you for clarification. I don't see the story the same way. gfx-hal is already pretty much Vulkan, just a bit rustier (borrowing, move semantics, and references instead of pointers and opaque handles). You could just have used gfx-hal and have access to all platforms, while benefiting gfx-rs community with your feedback.

Truth is that @omni-viral found gfx-hal inconvenient for their needs, hence the departure. I don't see it temporary. The expressed solution (in https://github.com/rustgd/rendy/pull/12#issuecomment-434338491) is basically - solve it on the Ash level, so that Amethyst wouldn't care about gfx-rs APIs.

Tight Ash (and Vulkano) integration is certainly one of our goals, but it's not going to be the most performant path at the end of the day, so we'd like Rust projects which really need performance (like Amethyst and WebRender) to work with gfx-hal directly.

Moxinilian · 2018-10-31T01:37:21Z

There is enough motivation within the Amethyst team to make sure hal will be supported by rendy, most notably in the perspective of WebGL (and eventually WebGPU). From what has been discussed on Discord, it seems like first class hal is planned in the future.

If I misunderstood, then this is going to be a much larger issue considering Amethyst's next step after mobile support will be web technologies, in which gfx-hal would help in a significant way.

kvark · 2018-10-31T01:41:40Z

@Moxinilian FYI, this is a very poor communication on the Amethyst side. Your team discussed something on discord (which we don't even have any visibility into, since we use Gitter) and decided to drop gfx-hal without any heads up to our team, without even bothering to explain this in PR description... We hope that in the future, if there is any news of that scale, we'd know about it from you.

Moxinilian · 2018-10-31T01:43:49Z

Yes, I agree. If I recall correctly, an issue is being written to be posted on the gfx issue tracker regarding the reasons for such a decision. I still believe that supporting hal from the ground up would have been better, but I am not involved enough in rendering to have a meaningful opinion.

Moxinilian · 2018-10-31T02:30:15Z

We handled this very poorly, and apologize to the entire gfx-hal team. It was inexcusable, and we will take steps internally to ensure it doesn't happen again. We would like to re-open a dialog on this with the gfx-hal team, if they are ok with it.

zakarumych · 2018-10-31T08:12:20Z

@kvark Sorry I didn't explained in detail why I abandoned hal in this PR. I also sad for that I didn't state my opinion clearly in gfx-rs/gfx#2206 😭

Correct me if I'm wrong.
As I understand it the main idea of hal API to stay is that it is rustier and reduces boxing overhead over non-Vulkan backends. But I also see calling overhead in hal's API for Vulkan backend which you underestimate (I think). Hence I'd really like to see ash's API implemented for hal's backends (notably metal backend which is awesome 👍).

Sorry. It's hard for me to express sophisticated stuff in language I barely know. It took me 15 minutes to write this comment 😅

Zakarum added 13 commits October 24, 2018 13:48

Move memory to ash

538956a

Move resources to ash

e1da76a

Move chains to ash

fecbd10

Move command to ash

94b2c7f

Init ash

a3147b7

Macos surface

1ac6112

Target creation. WIP

85e1e01

Vertices and meshes

a4f2761

wsi

e222f4a

Move wsi code to wsi

e475cce

Split crates more

a561216

fmt

93ba27e

Simple example WIP

0dfd6ea

LucioFranco reviewed Oct 29, 2018

View reviewed changes

Moving with Simple example

b21526d

dotellie reviewed Oct 29, 2018

View reviewed changes

glsl to spirv conversion at build-time

871902b

Rhuagh reviewed Oct 30, 2018

View reviewed changes

Zakarum added 2 commits October 30, 2018 15:55

Use shaderc

c6ba41b

Implementing methods for command pools and buffers

db7494d

Submission and presentation. Abstract few more things.

332b0a6

Unsafe documentation.

1a07373

torkleyy mentioned this pull request Oct 31, 2018

Backend of rendy #13

Closed

Zakarum added 2 commits November 2, 2018 20:03

Stress

7ac912b

fmt

ae26ee6

zakarumych changed the title ~~Getting rendy ready~~ What if Ash Nov 4, 2018

zakarumych closed this Nov 13, 2018

zakarumych deleted the ash branch November 30, 2018 11:10

What if Ash #12

What if Ash #12

Conversation

zakarumych commented Oct 28, 2018 • edited Loading

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotellie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Oct 29, 2018

Choose a reason for hiding this comment

zakarumych commented Oct 30, 2018 • edited Loading

Moxinilian commented Oct 31, 2018 • edited Loading

kvark commented Oct 31, 2018

Moxinilian commented Oct 31, 2018 • edited Loading

kvark commented Oct 31, 2018

Moxinilian commented Oct 31, 2018

Moxinilian commented Oct 31, 2018

zakarumych commented Oct 31, 2018

zakarumych commented Oct 28, 2018 •

edited

Loading

zakarumych commented Oct 30, 2018 •

edited

Loading

Moxinilian commented Oct 31, 2018 •

edited

Loading

Moxinilian commented Oct 31, 2018 •

edited

Loading