-
Notifications
You must be signed in to change notification settings - Fork 10
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
(WIP) Update to bevy 0.11 #21
Conversation
"bevy_asset", | ||
"bevy_render", | ||
"bevy_pbr", | ||
"ktx2", | ||
"tonemapping_luts", | ||
"zstd", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migration guide: https://bevyengine.org/learn/migration-guides/0.10-0.11/#change-default-tonemapping-method
without these, everything was pink!
Cargo.toml
Outdated
"bevy_scene", | ||
"bevy_winit", | ||
"png", | ||
"x11", | ||
] } | ||
bevy_mod_gltf_patched = "0.2" | ||
bevy_mod_gltf_patched = { git = "https://github.com/luggage66/bevy_mod_gltf_patched/", branch = "bevy-0.11" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed this to compile the hollow example.. this is a PR on bevy_mod_gltf_patched so.. obviously once that's merged it can be just updated to the latest version.
.init_resource::<DrawFunctions<StencilOutline>>() | ||
.init_resource::<DrawFunctions<OpaqueOutline>>() | ||
.init_resource::<DrawFunctions<TransparentOutline>>() | ||
.init_resource::<OutlinePipeline>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here (and similar add_systems/add_plugins changes) was following the migration guide, including this line but I wanted to point out this one
https://bevyengine.org/learn/migration-guides/0.10-0.11/#webgpu-support
this line actually got removed here and is now done in the finish function on line 282
@@ -248,16 +266,10 @@ impl Plugin for OutlinePlugin { | |||
.get_sub_graph_mut(bevy::core_pipeline::core_3d::graph::NAME) | |||
.unwrap(); | |||
draw_3d_graph.add_node(OUTLINE_PASS_NODE_NAME, node); | |||
draw_3d_graph.add_slot_edge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
// Run after main 3D pass, but before UI psss | ||
draw_3d_graph.add_node_edge( | ||
bevy::core_pipeline::core_3d::graph::node::MAIN_PASS, | ||
bevy::core_pipeline::core_3d::graph::node::END_MAIN_PASS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is due to this
it's listed under the 0.11 update https://bevyengine.org/news/bevy-0-11/#rendering but not in the migration guide.
As I mentioned in the PR description, when I had this set to START_MAIN_PASS, sometimes while running the examples I didn't get outlines at all. END_MAIN_PASS works better but I did notice a bit more transparency than usual in the shapes example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure using END_MAIN_PASS
is identical to the old MAIN_PASS
in terms of render graph, so this might be a different problem. I'm not sure what that is though.
@@ -128,8 +128,6 @@ pub(crate) struct OutlineNode { | |||
} | |||
|
|||
impl OutlineNode { | |||
pub(crate) const IN_VIEW: &'static str = "view"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes are again due to this
@@ -1,5 +1,6 @@ | |||
#import bevy_pbr::mesh_view_bindings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these shader changes were due to this
https://bevyengine.org/learn/migration-guides/0.10-0.11/#improve-shader-import-model
I'm not as experienced with these shaders, so I wouldn't be surprised if there are changes here that "work" but may only work in the cases I've been testing.
@@ -232,27 +231,19 @@ impl SpecializedMeshPipeline for OutlinePipeline { | |||
self.mesh_pipeline.view_layout_multisampled.clone() | |||
}]; | |||
let mut buffer_attrs = vec![Mesh::ATTRIBUTE_POSITION.at_shader_location(0)]; | |||
let mut vertex_defs = vec![ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect these are also related to the shader imports... What I was running into errors like this for the both of them
src/outline.wgsl: multiple inconsistent shader def values: 'MAX_DIRECTIONAL_LIGHTS'
when I removed these, the errors went away. I suspect that they just are already in scope now and since the value of these comes from bevy::pbr that it might be alright to just use what is already there?
src/pipeline.rs
Outdated
let mut fragment_defs = vec![]; | ||
bind_layouts.push( | ||
if mesh_layout.contains(Mesh::ATTRIBUTE_JOINT_INDEX) | ||
&& mesh_layout.contains(Mesh::ATTRIBUTE_JOINT_WEIGHT) | ||
{ | ||
vertex_defs.push(ShaderDefVal::from("SKINNED")); | ||
vertex_defs.push("MESH_BINDGROUP_1".into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The animated_fox example was failing until I added this...
Shader global ResourceBinding { group: 2, binding: 1 } is not available in the layout pipeline layout
I referenced this a bit and just added this MESH_BINDGROUP_1 here and that was enough to make everything work for that example.
This is one change that:
- Not sure if this should be done here
- Not sure if this works for all cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked back at this and it looks completely fine. the draw command in draw.rs
uses SetMeshBindGroup<1>
meaning that it will always use that group,
bevy puts mesh bindings in 2 because StandardMaterial
uses 1. this shader def was created specifically for cases like ours where someone wrote their own mesh pipeline that doesn't use StandardMaterial
. case in point, you can see its use in the official mesh instancing example.
the reason this used to work on the previous version is because this module's old shader sets it to 1 manually. I've since replaced this definition with raw imports of bevy_pbr::skinning
and bevy_pbr::morph
which need MESH_BINDGROUP_1
to function properly.
src/pipeline.rs
Outdated
buffer_attrs.push(Mesh::ATTRIBUTE_JOINT_INDEX.at_shader_location(2)); | ||
buffer_attrs.push(Mesh::ATTRIBUTE_JOINT_WEIGHT.at_shader_location(3)); | ||
self.mesh_pipeline.skinned_mesh_layout.clone() | ||
self.mesh_pipeline.mesh_layouts.skinned.clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These last two lines were due to this https://bevyengine.org/learn/migration-guides/0.10-0.11/#add-morph-targets
And, I suspect this is where handling of morph targets would take place.
thanks for doing most of the fixing for me. I added a PR to yours which should finish this by resolving morph targets. I also made some appropriate modifications to that shader so that the bind group locations for skinned meshes are at locations 5 and 6 instead of 2 and 3. I think they've changed. I'm not sure if you were able to get the old locations to work on your machine but they definitely didn't work for me. finally, I couldn't compile the
|
Morph targets
Hi @ramirezmike and @zainthemaynnn, Thanks for working on this in my absence. I will review and make a new release as soon as possible. |
@komadori just note that morph targets will not render properly until this other PR. the first one only allowed it to run without runtime errors. if that's not merged by the time you are done reviewing I can make a separate one. |
@zainthemaynnn Thanks, I have merged your additional PR into my staging branch: #22 |
Superseded by PR #22. |
I got this to a state where it's working well enough for my purposes but will likely need a couple tweaks.
Major concerns:
this fork of bevy_mod_gltf_patchedthis fork of bevy_mod_gltf_patched until it gets updatedDoesn't support morph targets (maybe could be handled as a separate issue?)Most of the rest of the changes were from following the migration guide