Skip to content
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

wasm32: non-spirv shader specialization #843

Merged
merged 5 commits into from
Nov 15, 2020
Merged

Conversation

mrk-its
Copy link
Member

@mrk-its mrk-its commented Nov 11, 2020

It implements shader specialization for wasm32 (where spirv is not available), by adding preprocessor's #define statements to shader source.

@mrk-its
Copy link
Member Author

mrk-its commented Nov 11, 2020

The one thing: maybe we should change get_spirv_shader method name to get_specialized_shader or so - because for wasm32 we are returning modified shader source instead of spirv.

stage: self.stage,
}
}

#[cfg(target_arch = "wasm32")]
pub fn get_spirv_shader(&self, macros: Option<&[String]>) -> Shader {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to future-proof this for an eventual WebGPU backend if possible. I don't want to conflate "wasm" with "webgl"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(where i think we would use the other variant of this function)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also don't like these target_arch = "wasm32" checks here. Maybe we should check presence of some spirv feature or so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or delegate this to RenderResourceContext too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that seems reasonable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done - shader specialization delegated to RenderResourceContext.get_specialized_shader

@mrk-its mrk-its force-pushed the wasm_shaders branch 2 times, most recently from 64b3c31 to 434a3e6 Compare November 12, 2020 00:56
@mrk-its mrk-its marked this pull request as draft November 12, 2020 01:23
@mrk-its mrk-its marked this pull request as ready for review November 12, 2020 01:36
@memoryruins memoryruins added O-Web Specific to web (WASM) builds A-Rendering Drawing game state to the screen labels Nov 12, 2020
let macros: Vec<String> = macros
.unwrap_or(&[])
.iter()
.chain((&["WGPU".to_string()]).iter())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think id prefer to not insert shader defs for backends (as that encourages writing shaders that are backend-specific). if you need to do that for WebGL backend shaders, could you instead use something like:

# ifdef WEBGL
// do webgl things
# endif

# ifndef WEBGL
// do default bevy things
# endif

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@cart cart merged commit 515d750 into bevyengine:master Nov 15, 2020
@ambeeeeee ambeeeeee mentioned this pull request Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen O-Web Specific to web (WASM) builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants