bevy icon indicating copy to clipboard operation
bevy copied to clipboard

add globals to mesh view bind group

Open IceSentry opened this issue 3 years ago • 8 comments
trafficstars

Objective

  • It's often really useful to have access to the time when writing shaders.

Solution

  • Add a UnifformBuffer in the mesh view bind group
  • This buffer contains the time, delta time and a wrapping frame count

https://user-images.githubusercontent.com/8348954/180130314-97948c2a-2d11-423d-a9c4-fb5c9d1892c7.mp4


Changelog

  • Added a GlobalsUniform at position 9 of the mesh view bind group

Notes

The implementation is currently split between bevy_render and bevy_pbr because I was basing my implementation on the ViewPlugin. I'm not sure if that's the right way to structure it.

I named this globals instead of just time because we could potentially add more things to it.

References in other engines

IceSentry avatar Jul 21 '22 04:07 IceSentry

I used this PR for my animated shader: https://youtu.be/BanMIX3_GiU

HackerFoo avatar Aug 07 '22 01:08 HackerFoo

I'm very much in favour of this, i know at least Unity offers this out of the box, I would imagine other engines do too, but I haven't checked.

I think the percentage of users that would need this in a shader is higher than the percentage of users that can easily understand how to implement it themselves, since it requires getting to work at a lower level than deriving AsBindGroup for a custom material (and the very few other requirements since 0.8).

On thing I would consider: How does this interact with the animate_shader example? On one hand it kind of makes it redundant, because extracting time is the aspect of that example that is the most needed by the average user, on the other hand the example shows how to manually set the bind group in a custom draw command, which is the most interesting bit, but for more advanced users.

Maybe the solution is to have a different example that shows setting the bing group for a different functionality, and adapt animate_shader to use the solution from this PR? (I guess only the latter is needed for this PR). Otherwise I think it might be confusing, but I'm not sure what you people think.

wilk10 avatar Aug 31 '22 08:08 wilk10

I'll update the animate_shader example soon to reflect the changes and after some quick discussion with @superdump we agreed that we indeed need lower level examples instead of relying on examples showcasing multiple things. These will be added in a separate PR. See https://github.com/bevyengine/bevy/issues/5843

IceSentry avatar Aug 31 '22 15:08 IceSentry

I added the frame count as a wrapping u32. Unfotunately, I can't rely on the FrameTimeDiagnosticsPlugin to be present, since it's not part of DefaultPlugins, so I just used a Local that I increment with wrapping_add(1) in the prepare system

IceSentry avatar Sep 12 '22 20:09 IceSentry

I'm not sure what I think of the name Globals. I wanted to say GlobalUniforms but then we'd need GlobalUniformsUniform. :)

Yeah, I was hoping somebody would have proposed a better name. I guess since this is only for time stuff this could just be a TimeUniform. Most engine do seem to treat time as something separate.

IceSentry avatar Sep 13 '22 22:09 IceSentry

something around frame metadata rather than globals?

mockersf avatar Sep 13 '22 22:09 mockersf

The time since startup value isn't really related to the frame though, since it's just the time since startup (or at least, the time since the last wrap period). Godot, unreal and unity all seem to put this under Time.

IceSentry avatar Sep 13 '22 23:09 IceSentry

The time since startup value isn't really related to the frame though

Oh you mean the frame start time 😄

mockersf avatar Sep 13 '22 23:09 mockersf

I rebased and used the now merged wrapping time api. This should be ready for a final review pass I think.

IceSentry avatar Sep 24 '22 01:09 IceSentry