[Merged by Bors] - Migrate to encase from crevice #4339

teoxoy · 2022-03-26T10:44:12Z

Objective

Unify buffer APIs
Also see Migrate to encase from crevice for uniform and storage buffer data formatting #4272

Solution

Replace vendored crevice with encase

Changelog

Changed StorageBuffer
Added DynamicStorageBuffer
Replaced UniformVec with UniformBuffer
Replaced DynamicUniformVec with DynamicUniformBuffer

Migration Guide

`StorageBuffer`

removed set_body(), values(), values_mut(), clear(), push(), append()
added set(), get(), get_mut()

`UniformVec` -> `UniformBuffer`

renamed uniform_buffer() to buffer()
removed len(), is_empty(), capacity(), push(), reserve(), clear(), values()
added set(), get()

`DynamicUniformVec` -> `DynamicUniformBuffer`

renamed uniform_buffer() to buffer()
removed capacity(), reserve()

superdump

This is a naturally-big PR, and so I have a bunch of comments, but I do think it is looking good. :) Great stuff!

superdump · 2022-05-04T13:25:20Z

crates/bevy_pbr/Cargo.toml

@@ -28,3 +28,4 @@ bevy_window = { path = "../bevy_window", version = "0.8.0-dev" }
 bitflags = "1.2"
 # direct dependency required for derive macro
 bytemuck = { version = "1", features = ["derive"] }
+encase = { git = "https://github.com/teoxoy/encase", features = ["glam"] }


Suggested change

encase = { git = "https://github.com/teoxoy/encase", features = ["glam"] }

encase = { git = "https://github.com/teoxoy/encase", features = ["glam"] }

superdump · 2022-05-04T13:26:30Z

crates/bevy_pbr/src/pbr_material.rs

@@ -291,12 +289,15 @@ impl RenderAsset for StandardMaterial {
            flags: flags.bits(),
            alpha_cutoff,
        };
-        let value_std140 = value.as_std140();
+
+        let byte_buffer = [0; <StandardMaterialUniformData as encase::Size>::SIZE.get() as usize];


I think I'd prefer to be explicit that this is u8 if it is:

Suggested change

let byte_buffer = [0; <StandardMaterialUniformData as encase::Size>::SIZE.get() as usize];

let byte_buffer = [0u8; <StandardMaterialUniformData as encase::Size>::SIZE.get() as usize];

superdump · 2022-05-04T13:29:42Z

crates/bevy_pbr/src/render/light.rs

 pub struct GpuLights {
-    // TODO: this comes first to work around a WGSL alignment issue. We need to solve this issue before releasing the renderer rework


superdump · 2022-05-04T13:35:36Z

crates/bevy_pbr/src/render/light.rs

+    data: Box<[UVec4; ViewClusterBindings::MAX_UNIFORM_ITEMS]>,
+}
+
+const _: () = assert!(<GpuClusterLightIndexListsUniform as encase::Size>::SIZE.get() <= 16384);


This looks kind of magical. Is this just a way of making a compile-time assertion? I'm guessing encase::Size::SIZE.get() is a constant function so can be evaluated at compile time? Checks... yup. Interesting. It looks weird at first, but it is nice to have it as a compile-time assertion. Maybe add a comment to clarify?

Suggested change

const _: () = assert!(<GpuClusterLightIndexListsUniform as encase::Size>::SIZE.get() <= 16384);

// NOTE: Assert at compile time that the GpuClusterLightIndexListsUniform size is within uniform buffer size limits

const _: () = assert!(<GpuClusterLightIndexListsUniform as encase::Size>::SIZE.get() <= 16384);

We don't have to solve it here, but the maximum uniform buffer binding size is platform-/API-/GPU-dependent so I guess ultimately we would want to not use slices for these things but rather use Vecs and 'make the most of the available space' so to speak. We can do that separately though in my opinion.

Yeah, it's a compile time assertion to make sure whenever there are changes to the struct its size will still be less than 16384.
I intended to add a comment but in the end missed it.

superdump · 2022-05-04T13:38:05Z

crates/bevy_pbr/src/render/mesh.rs

+                        joints: Box::new(entity_joints.try_into().unwrap()),
+                    },),
+                ));
+            }


Did you benchmark this in any way? I know @james7132 put quite some effort into optimising this so we need to be careful with it.

I haven't but it's a copy-paste from the previous SkinnedMeshJoints::build with the only change being that now each entity has its own Vec of joints.

Would I benchmark it via the custom_skinned_mesh example by tracking the FPS?

Oh, that line is wrong. What I meant to do is: entity_joints.into_boxed_slice().try_into().unwrap().
I'm getting around 1000FPS on master and around 900-950 on this branch (with the change above).

Any ideas how to improve it?

This really regresses performance, in my test with 500+ skinned meshes the extract_skinned_meshes now takes 24ms+ per frame, before this PR it was only 1.5 - 2ms.

Can we not continue using a single vec like it was before?

Do we have an in-repo test for the performance of this system? If not, what is a simple and minimal way of stressing it? Could we load one skinned Mesh asset and spawn it many times then animate them all?

I ended up reverting the SkinnedMesh changes and added a note about the usage of BufferVec with the help of @superdump.

superdump · 2022-05-04T14:07:16Z

crates/bevy_sprite/src/mesh2d/color_material.rs

@@ -145,12 +143,15 @@ impl RenderAsset for ColorMaterial {
            color: material.color.as_linear_rgba_f32().into(),
            flags: flags.bits(),
        };
-        let value_std140 = value.as_std140();
+
+        let byte_buffer = [0; <ColorMaterialUniformData as encase::Size>::SIZE.get() as usize];


Suggested change

let byte_buffer = [0; <ColorMaterialUniformData as encase::Size>::SIZE.get() as usize];

let byte_buffer = [0u8; <ColorMaterialUniformData as encase::Size>::SIZE.get() as usize];

superdump · 2022-05-04T14:09:31Z

deny.toml

 ]

 [licenses]
 unlicensed = "deny"
 copyleft = "deny"
 allow = [
    "MIT",
+    "MIT-0",


MIT-0 appears to be MIT with no attribution requirement. I don't think this will be a controversial addition at all, but any license addition is noteworthy and so... @cart ?

I saw you guys already had 0BSD which is essentially the same.

Yeah this is pretty much what we're doing with Bevy (and what rust does). I'm cool with this.

superdump · 2022-05-04T14:09:50Z

examples/shader/custom_vertex_attribute.rs

@@ -87,8 +87,13 @@ impl RenderAsset for CustomMaterial {
        (render_device, material_pipeline): &mut SystemParamItem<Self::Param>,
    ) -> Result<Self::PreparedAsset, PrepareAssetError<Self::ExtractedAsset>> {
        let color = Vec4::from_slice(&extracted_asset.color.as_linear_rgba_f32());
+
+        let byte_buffer = [0; <Vec4 as encase::Size>::SIZE.get() as usize];


Suggested change

let byte_buffer = [0; <Vec4 as encase::Size>::SIZE.get() as usize];

let byte_buffer = [0u8; <Vec4 as encase::Size>::SIZE.get() as usize];

superdump · 2022-05-04T14:10:51Z

examples/shader/shader_material.rs

@@ -73,8 +73,13 @@ impl RenderAsset for CustomMaterial {
        (render_device, material_pipeline): &mut SystemParamItem<Self::Param>,
    ) -> Result<Self::PreparedAsset, PrepareAssetError<Self::ExtractedAsset>> {
        let color = Vec4::from_slice(&extracted_asset.color.as_linear_rgba_f32());
+
+        let byte_buffer = [0; <Vec4 as encase::Size>::SIZE.get() as usize];


Suggested change

let byte_buffer = [0; <Vec4 as encase::Size>::SIZE.get() as usize];

let byte_buffer = [0u8; <Vec4 as encase::Size>::SIZE.get() as usize];

Also, this seems to be a very common pattern that is possibly worth a small helper function?

Indeed (for small writes).

It seems it can't be made into a generic function because you can't (yet) use associated constants in const expressions (for the array length).

Ok, no problem. It's not a big deal.

superdump · 2022-05-04T14:11:06Z

examples/shader/shader_material_glsl.rs

@@ -70,8 +68,13 @@ impl RenderAsset for CustomMaterial {
        (render_device, material_pipeline): &mut SystemParamItem<Self::Param>,
    ) -> Result<Self::PreparedAsset, PrepareAssetError<Self::ExtractedAsset>> {
        let color = Vec4::from_slice(&extracted_asset.color.as_linear_rgba_f32());
+
+        let byte_buffer = [0; <Vec4 as encase::Size>::SIZE.get() as usize];


Suggested change

let byte_buffer = [0; <Vec4 as encase::Size>::SIZE.get() as usize];

let byte_buffer = [0u8; <Vec4 as encase::Size>::SIZE.get() as usize];

exjam · 2022-05-05T16:00:36Z

crates/bevy_macro_utils/src/lib.rs

+            .maybe_get_path(BEVY)
+            .map(|bevy_path| {
+                let mut segments = bevy_path.segments;
+                segments.push(BevyManifest::parse_str("render"));


bevy::render::render_resource::encase
segments.push(BevyManifest::parse_str("render"));
segments.push(BevyManifest::parse_str("render_resource"));

exjam · 2022-05-05T16:01:02Z

crates/bevy_macro_utils/src/lib.rs

+                }
+            })
+            .or_else(|| bevy_manifest.maybe_get_path(BEVY_RENDER))
+            .map(|bevy_render_path| {


This second map needs to be inside the or_else, otherwise it gets appended to the end of the first code path above using BEVY.

Indeed, thanks!

use UniformComponentPlugin for SkinnedMeshUniform

cart

I'm generally very on board for this. Just a few comments around naming / type exports. I'm specifically interested in @teoxoy's thoughts here. I don't want to merge until we've come to an agreement, but I won't block merging on upstream changes to encase, provided we're on the same page.

cart · 2022-05-16T21:38:09Z

crates/bevy_render/src/render_resource/mod.rs

@@ -44,6 +44,9 @@ pub use wgpu::{
    VertexStepMode,
 };

-pub use bevy_crevice::*;
+pub mod encase {
+    pub use bevy_encase_derive::ShaderType;


I'd prefer to export ShaderType (both the derive and the trait) directly at the render_resource level. This will be a relatively common derive, the longer "nested" path is regrettable, and the context of encase isn't something most bevy users should be thinking about.

I'd also prefer it if we renamed this module from encase to shader_type, given that there will almost certainly be cases where users import those types directly. From the perspective of bevy users, using the encase name is less helpful. I understand that this is a form of debranding, but I think we could solve the issue of "visible credit" in a way that doesn't affect UX (ex: adding encase to the bevy readme like we do for our other core libs. the readme is almost certainly more visible than a niche import). I'd like to hash out this point before merging, but I won't block this PR on making that work (as I believe doing that would require adjusting the encase derive impl).

Or maybe alternatively, do the full encase re-export (under the encase name), but then at the render_resource level export everything that will cover normal bevy use cases. This also gives us full control over the namespace / gives us the ability to type alias stuff without stomping on naming assumptions baked into encase.

I've been experimenting with moving the ShaderType export "out" of the blanket encase export, and sadly if you do this theres a "conflict" error:

// in render_resource/mod.rs pub use bevy_encase_derive::ShaderType; pub use encase::ShaderType;

This can be fixed by either:

Moving the blanket export up one level (merging it into render_resource). This doesn't seem tenable though because of present and future type conflicts (ex our DynamicUniformBuffer conflicts with encase's).

Remove encase's derive export. Obviously this also isn't tenable because encase needs to be viable as a standalone library.

Add an optional feature flag to disable encase's derive export.

None of those are ideal. (3) feels like a reasonable compromise, but thats solely @teoxoy's call.

The following seems to work

pub use self::encase::{ShaderType, Size as ShaderSize};

I think having the full encase re-export and then re-exporting the ShaderType and ShaderSize from it at the root of render_resource is a good approach since it provides the flexibilty of having the full re-export and also the ease of use for ShaderType and ShaderSize.

I'll push a commit with this change - let me know what you think!

Sadly that only currently compiles because the examples are accessing the encase path:

Trying to use the render_resource::ShaderType derive results in the the encase version of the derive being used.

Arg wait nevermind my bad. I was seeing a different error message! The examples do need to be updated to use the new paths, but everything appears to be fine. I'll push the tweaks.

Ah, yeah I somehow missed those. Thanks!

cart · 2022-05-16T21:38:53Z

examples/shader/custom_vertex_attribute.rs

@@ -87,8 +87,13 @@ impl RenderAsset for CustomMaterial {
        (render_device, material_pipeline): &mut SystemParamItem<Self::Param>,
    ) -> Result<Self::PreparedAsset, PrepareAssetError<Self::ExtractedAsset>> {
        let color = Vec4::from_slice(&extracted_asset.color.as_linear_rgba_f32());
+
+        let byte_buffer = [0u8; Vec4::SIZE.get() as usize];


Given that Size is now a part of our "global public api", I'd prefer it if this was ShaderSize::SHADER_SIZE. SIZE feels too generic / too easy to conflate with "size of Vec4 type on the CPU".

I agree with the ShaderSize rename however I'm not sure about ShaderSize::SHADER_SIZE vs ShaderSize::SIZE yet. What do you think? If SHADER_SIZE is not a hard requirement, maybe we could go with the re-export Size as ShaderSize for now?

Definitely not a hard requirement! The type alias is perfectly fine for this pr. The rename from Size->ShaderSize does make it a bit more self documenting on import, but in the actual code I still think X::SIZE is a liability. SIZE is just too generic of a concept. Imo the extra clarity of X::SHADER_SIZE would benefit all encase users (not just bevy).

It's a solid argument. I used to use the full disambiguation syntax (i.e. <x as encase::Size>::SIZE) but later removed since it's not really ergonomic. I'll rename encase's Size::SIZE to ShaderSize::SHADER_SIZE in the next major release; let me know if you'd like the change to happen sooner since I don't have an ETA on that.

Awesome. Much appreciated! Definitely no rush. I'd prefer it if it happened before Bevy 0.8 (mid july) to avoid breaking people twice, but even if that doesn't work out its not the biggest deal in the world.

cart · 2022-05-18T20:58:27Z

bors r+

cart · 2022-05-18T21:03:09Z

Very nice work! I think bevy graphics devs/users are gonna love these changes ❤️

# Objective - Unify buffer APIs - Also see #4272 ## Solution - Replace vendored `crevice` with `encase` --- ## Changelog Changed `StorageBuffer` Added `DynamicStorageBuffer` Replaced `UniformVec` with `UniformBuffer` Replaced `DynamicUniformVec` with `DynamicUniformBuffer` ## Migration Guide ### `StorageBuffer` removed `set_body()`, `values()`, `values_mut()`, `clear()`, `push()`, `append()` added `set()`, `get()`, `get_mut()` ### `UniformVec` -> `UniformBuffer` renamed `uniform_buffer()` to `buffer()` removed `len()`, `is_empty()`, `capacity()`, `push()`, `reserve()`, `clear()`, `values()` added `set()`, `get()` ### `DynamicUniformVec` -> `DynamicUniformBuffer` renamed `uniform_buffer()` to `buffer()` removed `capacity()`, `reserve()` Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2022-05-18T21:33:00Z

Pull request successfully merged into main.

Build succeeded:

…es (#4816) # Objective fixes #4811 (caused by #4339 [[exact change](https://github.com/bevyengine/bevy/pull/4339/files#diff-4bf3ed03d4129aad9f5678ba19f9b14ee8e3e61d6f6365e82197b01c74468b10R712-R721)] - where the buffer type has been changed from `UniformVec` to `BufferVec`) ## Solution Use uniform buffer usage for `SkinnedMeshUniform` instead of all usages due to the `Default` derive.

…es (bevyengine#4816) # Objective fixes bevyengine#4811 (caused by bevyengine#4339 [[exact change](https://github.com/bevyengine/bevy/pull/4339/files#diff-4bf3ed03d4129aad9f5678ba19f9b14ee8e3e61d6f6365e82197b01c74468b10R712-R721)] - where the buffer type has been changed from `UniformVec` to `BufferVec`) ## Solution Use uniform buffer usage for `SkinnedMeshUniform` instead of all usages due to the `Default` derive.

# Objective - Unify buffer APIs - Also see bevyengine#4272 ## Solution - Replace vendored `crevice` with `encase` --- ## Changelog Changed `StorageBuffer` Added `DynamicStorageBuffer` Replaced `UniformVec` with `UniformBuffer` Replaced `DynamicUniformVec` with `DynamicUniformBuffer` ## Migration Guide ### `StorageBuffer` removed `set_body()`, `values()`, `values_mut()`, `clear()`, `push()`, `append()` added `set()`, `get()`, `get_mut()` ### `UniformVec` -> `UniformBuffer` renamed `uniform_buffer()` to `buffer()` removed `len()`, `is_empty()`, `capacity()`, `push()`, `reserve()`, `clear()`, `values()` added `set()`, `get()` ### `DynamicUniformVec` -> `DynamicUniformBuffer` renamed `uniform_buffer()` to `buffer()` removed `capacity()`, `reserve()` Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…es (bevyengine#4816) # Objective fixes bevyengine#4811 (caused by bevyengine#4339 [[exact change](https://github.com/bevyengine/bevy/pull/4339/files#diff-4bf3ed03d4129aad9f5678ba19f9b14ee8e3e61d6f6365e82197b01c74468b10R712-R721)] - where the buffer type has been changed from `UniformVec` to `BufferVec`) ## Solution Use uniform buffer usage for `SkinnedMeshUniform` instead of all usages due to the `Default` derive.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 26, 2022

alice-i-cecile added A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Mar 26, 2022

teoxoy force-pushed the encase branch from fbcaba9 to 04f0d8f Compare May 4, 2022 12:50

teoxoy marked this pull request as ready for review May 4, 2022 12:51

teoxoy force-pushed the encase branch from 04f0d8f to e81a412 Compare May 4, 2022 13:00

superdump requested changes May 4, 2022

View reviewed changes

exjam suggested changes May 5, 2022

View reviewed changes

teoxoy added 5 commits May 5, 2022 20:28

replace crevice with encase

2b3be6f

use UniformComponentPlugin for SkinnedMeshUniform

address comments

cdd0eea

re-export encase

67aa8fb

re-export encase's derive macro through a wrapper

746275c

add bevy_encase_derive to publish.sh

ce60917

teoxoy force-pushed the encase branch from 05a2aca to ce60917 Compare May 5, 2022 18:30

teoxoy added 3 commits May 5, 2022 22:11

use new encase release

7f1a21c

remove new no longer needed tests (missed this when rebased)

6e04b8c

revert changes to SkinnedMesh

df7437b

alice-i-cecile mentioned this pull request May 16, 2022

Update insta requirement from 0.16.1 to 1.14.0 #4349

Closed

cart reviewed May 16, 2022

View reviewed changes

re-export ShaderType and ShaderSize at render_resource root

fdd9d3f

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label May 18, 2022

alice-i-cecile added this to the Bevy 0.8 milestone May 18, 2022

alice-i-cecile removed the C-Feature A new feature, making something new possible label May 18, 2022

update paths

117175b

bors bot changed the title ~~Migrate to encase from crevice~~ [Merged by Bors] - Migrate to encase from crevice May 18, 2022

bors bot closed this May 18, 2022

infmagic2047 mentioned this pull request May 20, 2022

Panic on startup in web build #4811

Closed

johanhelsing mentioned this pull request May 20, 2022

Make animate_shader example work on WASM #4814

Closed

teoxoy deleted the encase branch May 20, 2022 20:32

teoxoy mentioned this pull request May 20, 2022

[Merged by Bors] - Use uniform buffer usage for SkinnedMeshUniform instead of all usages #4816

Closed

cart mentioned this pull request May 25, 2022

Consider breaking bevy_core up #2931

Open

superdump mentioned this pull request May 31, 2022

Migrate to encase from crevice for uniform and storage buffer data formatting #4272

Closed

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 4, 2022

teoxoy mentioned this pull request Jun 30, 2022

Rename Size::SIZE to ShaderSize::SHADER_SIZE teoxoy/encase#6

Merged

nicopap mentioned this pull request Sep 23, 2023

Move skin code to a separate module #9899

Merged

	encase = { git = "https://github.com/teoxoy/encase", features = ["glam"] }
	encase = { git = "https://github.com/teoxoy/encase", features = ["glam"] }

	let byte_buffer = [0; <StandardMaterialUniformData as encase::Size>::SIZE.get() as usize];
	let byte_buffer = [0u8; <StandardMaterialUniformData as encase::Size>::SIZE.get() as usize];

		pub struct GpuLights {
		// TODO: this comes first to work around a WGSL alignment issue. We need to solve this issue before releasing the renderer rework

	const _: () = assert!(<GpuClusterLightIndexListsUniform as encase::Size>::SIZE.get() <= 16384);
	// NOTE: Assert at compile time that the GpuClusterLightIndexListsUniform size is within uniform buffer size limits
	const _: () = assert!(<GpuClusterLightIndexListsUniform as encase::Size>::SIZE.get() <= 16384);

	let byte_buffer = [0; <ColorMaterialUniformData as encase::Size>::SIZE.get() as usize];
	let byte_buffer = [0u8; <ColorMaterialUniformData as encase::Size>::SIZE.get() as usize];

	let byte_buffer = [0; <Vec4 as encase::Size>::SIZE.get() as usize];
	let byte_buffer = [0u8; <Vec4 as encase::Size>::SIZE.get() as usize];

[Merged by Bors] - Migrate to encase from crevice #4339

[Merged by Bors] - Migrate to encase from crevice #4339

Conversation

teoxoy commented Mar 26, 2022 • edited Loading

Objective

Solution

Changelog

Migration Guide

StorageBuffer

UniformVec -> UniformBuffer

DynamicUniformVec -> DynamicUniformBuffer

superdump 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

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

cart 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

cart commented May 18, 2022

cart commented May 18, 2022

bors bot commented May 18, 2022

teoxoy commented Mar 26, 2022 •

edited

Loading

`StorageBuffer`

`UniformVec` -> `UniformBuffer`

`DynamicUniformVec` -> `DynamicUniformBuffer`