-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
improve shader import model #5703
Merged
Merged
Changes from 51 commits
Commits
Show all changes
67 commits
Select commit
Hold shift + click to select a range
727a700
use naga_oil
robtfm bdadb44
remove a few unused shader imports
robtfm 2813f77
add override example
robtfm d49bfd1
readme
robtfm cf36c73
add global default shader overrides
robtfm 32d6fd6
simplify assuming single render device
robtfm 98cdcdd
format
robtfm f120513
snake_case wgsl imports
robtfm e534bae
move builtin position into vertex structs
robtfm e5cbc2f
Merge branch 'main' into shader_modules
robtfm 9e9390d
Merge branch 'main' into shader_modules
robtfm 34f1c39
fix pbr_global_override demo
robtfm 6aad22d
cleanup overrides, use composer struct api
robtfm 7fcbbe4
Merge branch 'main' into shader_modules
robtfm 5d1f044
Merge branch 'main' into shader_modules
robtfm 0e74bb9
use crates.io
robtfm dc1ee82
Merge branch 'main' into shader_modules
robtfm 74c08ad
Merge branch 'main' into shader_modules
robtfm ab278d0
update for wgpu/naga/naga_oil version
robtfm da1b71e
Merge branch 'main' into shader_modules
robtfm 6f463b3
Merge branch 'main' into shader_modules
robtfm 40d64a2
Merge branch 'main' into shader_modules
robtfm 7304db3
Merge branch 'main' into shader_modules
robtfm cf245c5
Merge branch 'main' into shader_modules
robtfm a393728
repair animate_shader example
robtfm e165da9
module defs
robtfm e270ccc
Merge branch 'main' into shader_modules
robtfm 343c1c7
naga_oil v4
robtfm d988d4a
use `#from` in shaders
robtfm 3f1076a
Merge branch 'main' into shader_modules
robtfm ffc1371
fix new examples
robtfm 9cf33c2
Merge branch 'main' into shader_modules
robtfm cc3937b
Merge branch 'main' into shader_modules
robtfm 09f0313
cleanup load_internal_asset changes
robtfm 7a7977c
example comments
robtfm 4ab19b4
a != A
robtfm 94e0add
Merge branch 'main' into shader_modules
robtfm e49b95b
cleanup AssetPath handling
robtfm 8904f33
bump naga_oil v0.5
robtfm b76ae46
clippy
robtfm f5c1ac8
rename inverse_transpose_3x3
robtfm 13de5ac
Merge branch 'main' into shader_modules
robtfm 30b4350
Merge branch 'main' into shader_modules
robtfm 83aba93
remove `#else if` tests
robtfm 1c5fef3
fix pbr_global_override example
robtfm 34ed302
remove unsafe from example
robtfm 7e08a2d
Merge branch 'main' into shader_modules
robtfm 2b2184f
fix merge
robtfm fa9935d
work work
robtfm 1f8de78
remove patch
robtfm 50870f2
template pages
robtfm a386f7e
review comments
robtfm b1bc7be
Merge branch 'main' into shader_modules
robtfm d7e5f8b
undecorate items
robtfm 647517b
point to git branch
robtfm ea4c92e
fmt
robtfm ca40e07
fix load_internal_binary_asset
robtfm 94d47eb
Merge remote-tracking branch 'upstream/main' into shader_modules
robtfm 8304580
Merge remote-tracking branch 'upstream/main' into shader_modules
robtfm 27f2ee4
fmt
robtfm 689a7ce
update naga_oil url to bevyengine
robtfm 44340e5
Merge remote-tracking branch 'upstream/main' into shader_modules
robtfm 8326c7a
Merge remote-tracking branch 'upstream/main' into shader_modules
robtfm 6a72438
switch to crates.io
robtfm d2084bf
fix vertex_tangents accessors
robtfm 3b5598d
MeshVertexOutput.clip_position -> .position
robtfm d373df4
fix more examples
robtfm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,60 +1,51 @@ | ||
#import bevy_pbr::mesh_view_bindings | ||
#import bevy_pbr::mesh_bindings | ||
|
||
#import bevy_pbr::pbr_types | ||
#import bevy_pbr::utils | ||
#import bevy_pbr::clustered_forward | ||
#import bevy_pbr::lighting | ||
#import bevy_pbr::shadows | ||
#import bevy_pbr::fog | ||
#import bevy_pbr::pbr_functions | ||
#import bevy_pbr::pbr_ambient | ||
#from bevy_pbr::mesh_vertex_output import MeshVertexOutput | ||
#from bevy_pbr::mesh_view_bindings import view | ||
#from bevy_pbr::pbr_types import STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT | ||
#import bevy_pbr::pbr_functions as fns | ||
#from bevy_core_pipeline::tonemapping import tone_mapping | ||
|
||
@group(1) @binding(0) | ||
var my_array_texture: texture_2d_array<f32>; | ||
@group(1) @binding(1) | ||
var my_array_texture_sampler: sampler; | ||
|
||
struct FragmentInput { | ||
@builtin(front_facing) is_front: bool, | ||
@builtin(position) frag_coord: vec4<f32>, | ||
#import bevy_pbr::mesh_vertex_output | ||
}; | ||
|
||
@fragment | ||
fn fragment(in: FragmentInput) -> @location(0) vec4<f32> { | ||
let layer = i32(in.world_position.x) & 0x3; | ||
fn fragment( | ||
@builtin(front_facing) is_front: bool, | ||
mesh: ::MeshVertexOutput, | ||
) -> @location(0) vec4<f32> { | ||
let layer = i32(mesh.world_position.x) & 0x3; | ||
|
||
// Prepare a 'processed' StandardMaterial by sampling all textures to resolve | ||
// the material members | ||
var pbr_input: PbrInput = pbr_input_new(); | ||
var pbr_input: fns::PbrInput = fns::pbr_input_new(); | ||
|
||
pbr_input.material.base_color = textureSample(my_array_texture, my_array_texture_sampler, in.uv, layer); | ||
pbr_input.material.base_color = textureSample(my_array_texture, my_array_texture_sampler, mesh.uv, layer); | ||
#ifdef VERTEX_COLORS | ||
pbr_input.material.base_color = pbr_input.material.base_color * in.color; | ||
pbr_input.material.base_color = pbr_input.material.base_color * mesh.color; | ||
#endif | ||
|
||
pbr_input.frag_coord = in.frag_coord; | ||
pbr_input.world_position = in.world_position; | ||
pbr_input.world_normal = prepare_world_normal( | ||
in.world_normal, | ||
(pbr_input.material.flags & STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u, | ||
in.is_front, | ||
pbr_input.frag_coord = mesh.clip_position; | ||
pbr_input.world_position = mesh.world_position; | ||
pbr_input.world_normal = fns::prepare_world_normal( | ||
mesh.world_normal, | ||
(pbr_input.material.flags & ::STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u, | ||
is_front, | ||
); | ||
|
||
pbr_input.is_orthographic = view.projection[3].w == 1.0; | ||
pbr_input.is_orthographic = ::view.projection[3].w == 1.0; | ||
|
||
pbr_input.N = apply_normal_mapping( | ||
pbr_input.N = fns::apply_normal_mapping( | ||
pbr_input.material.flags, | ||
pbr_input.world_normal, | ||
mesh.world_normal, | ||
#ifdef VERTEX_TANGENTS | ||
#ifdef STANDARDMATERIAL_NORMAL_MAP | ||
in.world_tangent, | ||
mesh.world_tangent, | ||
#endif | ||
#endif | ||
in.uv, | ||
mesh.uv, | ||
); | ||
pbr_input.V = calculate_view(in.world_position, pbr_input.is_orthographic); | ||
pbr_input.V = fns::calculate_view(mesh.world_position, pbr_input.is_orthographic); | ||
|
||
return tone_mapping(pbr(pbr_input)); | ||
return ::tone_mapping(fns::pbr(pbr_input), ::view.color_grading); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is there a particular reason we can't use this syntax?
This is another "non-standard" (or at least, less common) thing that will rub some people the wrong way. We already must have knowledge of "live" module names to resolve imports, so we can decide if something is a module name or a symbol inside of it.
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 doable. it would mean braces can't be used in module names, but that's fine.
not sure about these. we use the imported module names to work out what modules need to be loaded (and to trigger loading them, for path imports). it would break confusingly if modules get loaded out of order (which can definitely happen with async io), or if you misname an imported item and it tells you the module can't be found.
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'm not sure I understand what's different about
#import foo::Bar
that makes it more complicated than#import foo::{Bar, Baz}
Also, couldn't the
*
just be syntax sugar that expands to the syntax with{Bar, Baz}
?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 a good point (and feels like its touching on the core issue).
How do we determine if
#import bevy_pbr::mesh_vertex_output
doesn't exist, or is just still loading?. Currently iirc we rely onassets.set_untracked
being called ahead of a given load? Am I right in thinking that module names don't work at all (or at least, not reliably) forserver_loaded_shader_asset_file -> server_loaded_shader_asset_file
deps? (both on main and in this pr?).If so thats a hole that needs filling. This is especially important because we almost certainly want to be able to pre-process shaders at development time (in the short term producing final WGSL, in the long term ideally producing per-platform compiled shaders). This implies resolving these dependencies using the asset system instead of at runtime. I think the new asset system would be able to handle this with "processed ids" (ex: this is how I plan to support UUIDs and other "stable" ids).
However the asset system won't have access to a "global shader identity cache" and it couldn't just listen for things to show up there. It would just be able to wait for a given stable id (such as
bevy_pbr::mesh_vertex_output
) to be resolved (or fail to resolve after all assets have been processed). So that does probably leave us with the need for an unambiguous syntax in this case. We could probably make stable id lookups fallible, but it would mean that shaders with#import bevy_pbr::mesh_vertex_output::foo
(where foo is a function name) would always be processed "last" and they would be blocking constantly on other work finishing ... not ideal.So I guess that does probably leave us in the unfortunate situation where module imports must be unambiguous with other symbol imports (unless anyone else has ideas).
In that case, I think my current preferred syntax is in the vein of:
Not sure how I feel about Option C. It has the interesting (and likely controversial) property of having rough parity with asset paths (with file extensions resolving the ambiguity)
(pretty sure that parity creates confusion but im still sorting out my feelings)
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 think Option A has my preference
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 prefer these over
from
variants because those lack symmetry with other imports. I like that everything reads as#import
operations followed by paths, rather than some being "import" operations (#import PATH
) and others being "from" operations (#from PATH import THINGS
).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 mentioned it a few months ago, but I think we could also consider using
#use
instead of#import
to mirror rust a bit more closely. There's the downside that people might assume it's exactly like rust of course, but I think it's worth considering.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.
yeah im pretty ok with
use
(rust parallels generally).use
reads nicelyThere 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.
for manually server-loaded shaders, it works fine in both main and in this pr. in both cases we preprocess the shader to extract the dependencies and wait for them to be loaded ... but this requires that we can identify dependencies without context (and disambiguate them from item imports).
i'm not sure if
AssetPath
(i.e.#import "module.wgsl"
) shader deps get auto-loaded on main, but they do in this pr.i'll give this a go