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

Adding push constants support #1369

Closed
wants to merge 4 commits into from

Conversation

Neo-Zhixing
Copy link
Contributor

This PR adds Push Constants support to the bevy rendering engine. This exposes the wgpu push constants API to the user.

However, we might want to coordinate with #547. Push Constants requires enabling the push constant feature and setting an appropriate resource limit. Therefore we might need to wait for #547, or maybe get this merged without enabling the push constants feature by default.

@Neo-Zhixing
Copy link
Contributor Author

Blocked on #1401

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@Neo-Zhixing
Copy link
Contributor Author

Unblocked by #547. This is now ready for review. @cart

@cart
Copy link
Member

cart commented Mar 3, 2021

I think this would benefit from a minimal example that proves the feature works / illustrates how to use it.

@Neo-Zhixing
Copy link
Contributor Author

Neo-Zhixing commented Mar 4, 2021

@cart This API isn't exactly user-facing, so I don't think it's a good idea to make a new example inside bevy. This PR is supposed to be the foundations for #1372, and we'd probably need to build a PushConstantsManager or something similar so that users could add data to the push constants when needed.

Please take a look at the following for an example of how this would work:

https://github.com/Neo-Zhixing/bevy_sky/blob/b49ac0520c751ab47936126fbeef828b44540c40/src/sky_node.rs#L139

https://github.com/Neo-Zhixing/bevy_sky/blob/b49ac0520c751ab47936126fbeef828b44540c40/src/procsky.frag#L10

https://github.com/Neo-Zhixing/bevy_sky/blob/b49ac0520c751ab47936126fbeef828b44540c40/examples/procedural_sky.rs#L8

@cart
Copy link
Member

cart commented Mar 5, 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.

Neo-Zhixing and others added 4 commits March 6, 2021 19:04
# Conflicts:
#	crates/bevy_render/src/pipeline/pipeline_layout.rs
#	crates/bevy_wgpu/src/wgpu_type_converter.rs
@alice-i-cecile
Copy link
Member

This can probably be closed with the rendering rework right? It'll be very stale, and seems like we should wait for a compelling use case.

@mockersf mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 1, 2021
@cart
Copy link
Member

cart commented Jul 1, 2021

The rework directly exposes the ability to enable push constants via gpu (because it uses wgpu directly), so I do think we can close this out.

@cart cart closed this Jul 1, 2021
@aloucks
Copy link
Contributor

aloucks commented Dec 20, 2021

It doesn't look like set_push_constants is exposed in the new TrackedRenderPass.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants