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

Update and extension of Adding push constants support #1369 #1733

Closed
wants to merge 8 commits into from

Conversation

mtsr
Copy link
Contributor

@mtsr mtsr commented Mar 22, 2021

This extends @Neo-Zhixing 's #1369 with shader-reflection and a (albeit contrived) example.

@Neo-Zhixing I hope you don't mind. I left your authorship of the rebased commits intact.

I'd be particularly interested in feedback on how to make a more useful or simple example. But since set_push_constants requires DrawContext, I'm not sure if there's a real alternative.

@Neo-Zhixing
Copy link
Contributor

This is really nice. Thank you for keeping my authorship.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Mar 22, 2021
@mtsr
Copy link
Contributor Author

mtsr commented Mar 22, 2021

Push constants would be user-facing in the sense that users could use them (like you are using them in the linked bevy_sky examples). For internal adoption, my only major concern is "web compatibility". I don't want to maintain two render paths yet (one for web and one for "native"). Imo we should have a real "motivating" use case before merging. If it can't be internal (for now), then I think it needs to be "allowing users to adopt push constants". And if thats true, we should probably have an example.

I'm looking at using push_constants for shadowmapping. That way I can pass the index of the pointlight and the face that needs to be rendered to the shadow map shader.

@mtsr
Copy link
Contributor Author

mtsr commented Mar 23, 2021

Although the limited compatibility concerns me too. I'll look into pushing that data into an aligned uniform as an alternative.

for push_constant_block in module.enumerate_push_constant_blocks(None).unwrap() {
let range = push_constant_block.offset..push_constant_block.size;
let mut stages = BindingShaderStage::NONE;
if shader_stage.contains(ReflectShaderStageFlags::VERTEX) {

Choose a reason for hiding this comment

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

This seems like it could be potentially non-exhaustive. Room for improved error handling?

@mtsr
Copy link
Contributor Author

mtsr commented Mar 23, 2021

@Neo-Zhixing Btw, if you're interested in putting in the work to get this approved, feel free to pull (some of) my commits into your PR and we can close this instead.

@mtsr mtsr closed this Apr 24, 2021
@alice-i-cecile alice-i-cecile mentioned this pull request May 22, 2022
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 C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants