-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
defer render systems until render resource context is ready #662
Conversation
4617455
to
f53645c
Compare
A couple of things:
|
Sorry for late response, I didn't notice your comment.
No problem, I've just reverted it (and temporary added these additional
No blocking, please :) But separate schedule for |
crates/bevy_render/src/lib.rs
Outdated
@@ -163,6 +156,22 @@ impl Plugin for RenderPlugin { | |||
shader::clear_shader_defs_system.system(), | |||
); | |||
|
|||
let mut schedule = Schedule::default(); |
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.
@cart what about that?
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.
Hmm it presents a couple of problems:
- Users can't insert their systems into the schedule (although thats solvable by inserting the schedule as a resource)
- It doesn't solve the problem for systems in the "normal schedule".
Its also worth pointing out that the wgpu backend currently "punts" this by using future::block_on(WgpuRenderer::new())
, which also probably won't work on the web.
I think we have a few options:
- Separate schedule that executes only when RenderResourceContext is ready (like what you're doing here)
- Fully delay normal schedule execution until the render context is ready (creates its own set of problems, as we'll likely want some systems like Time/Diagnostics to run during this process. And theres no harm in starting things like asset loads while we wait for rendering to initialize)
- Queue up render resource commands that are created before the context is ready (complicates the implementation)
(3) might actually be the right call here:
- It means we no longer need the public interface to be
Box<dyn RenderResourceContext>
. It could just be a backend-agnosticRenderResourceContext
that queues up commands, which is much nicer to consume (although it would make stack traces a bit less useful if/when a backend command panics). - Render backends could then just empty those command lists and process them as they see fit
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.
Hmm it presents a couple of problems:
- Users can't insert their systems into the schedule (although thats solvable by inserting the schedule as a resource)
I thought about adding some interface on RenderPlugin for adding additional systems for this schedule (even did some implementation), but it also doesn't seem like solution we want.
- It doesn't solve the problem for systems in the "normal schedule".
And doesn't solve above.
I think we have a few options:
- Separate schedule that executes only when RenderResourceContext is ready (like what you're doing here)
- Fully delay normal schedule execution until the render context is ready (creates its own set of problems, as we'll likely want some systems like Time/Diagnostics to run during this process. And theres no harm in starting things like asset loads while we wait for rendering to initialize)
- Queue up render resource commands that are created before the context is ready (complicates the implementation)
I see some other:
-
Register resource as Option<Box<dyn RenderResourceContext>> - RenderResoureContext will be None initialy and replaced with ready RenderResourceContext later. So each system needing this resource will have to deal with this optional RenderResourceContext (instead of checking if RenderResourceContext is ready - Option looks like a much safer way of checking this readiness)
-
This one requires some change of ECS behaviour:
do not call system if resource it requires is not registered
(now systems panic in such case). This way we would simply defer of registration of RenderResourceContext until is ready. It is better IMHO than disabling whole schedules. This may be also quite usefull fearture in regular (non-rendering-related) ECS use cases.
I thought a bit about (3) - but it is quite problematic. For example systems write to gpu buffers directly now, if you want to queue this you need to buffer everything in memory first, etc. It looks also like very huge refactor.
The more I'm thinking about it the more I like (5) :)
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.
@cart I've just pushed here implementation of (5) - take a look how little changes are required now. I also think regular ECS users would benefit from this new behaviour of systems. I see one problematic thing now - we had these panics where some plugin dependencies were not met, now plugin's systems will simply not run and there will be no information why. But it can be probably quite easily solved by declaring dependencies in plugins (some way) and checking that independently.
I tested this approach with bevy_webgl2 by creating RenderResourceContext & SharedBuffers resources after winit window is ready - and it works great.
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.
That is a good solve for this particular problem, but I am a bit worried that it comes at the cost of strictness everywhere else. Being able to assume that resources are "always there" is a nice constraint that I'm hesitant to lift. Things like "dependencies between systems" get hairy because now a system that must only run after another system does something / produces some output, now might run even though the dependency didnt run. This could either be an implicit dependency (ex: a system in STAGE_A runs before a system in STAGE_B, or two systems in the same stage having conflicting data access) or an explicit dependency (which we are planning on adding. ex: system_a depends on system_b).
Lol this is hard.
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've also tried to implement (4): Option<Box> - it is definitely better than is_ready() check, but still you must handle it everywhere.
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.
Maybe a "state transition" api is a good way to handle this problem. Ex: many folks want something like:
LOADING_STATE: run schedule that sets up resources
RUN_GAME_STATE: run normal game schedule
transition LOADING_STATE to RUN_GAME_STATE when asset_server.all_assets_loaded()
Maybe we could could have something like a RENDER_CONTEXT_INIT state that transitions to RUN_GAME_STATE when render_resource_context.is_ready()
?
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.
After changing webgl2 initialization a bit it seems I no longer need this (I'm assuming now that window is created on start of application). It still would be great to have this, but I can wait until these state transitions are available. Closing it now.
d0bf911
to
aa7486f
Compare
First of several patches making possible to create WebGL renderer for bevy.
This one allows to defer few bevy_render systems (
render_graph_schedule_executor_system
anddraw_render_pipelines_system
) untilrender_resource_context
is ready (to obtain webgl context cavas is needed, so we need to wait until window is created).Additionally it turns
mesh_resource_provider_system
andtexture_resource_system
into render graph nodes and removes theRENDER_RESOURCE
stage (as suggested in @cart's comment - I'm doing it here because these systems also need to be deferred).On
wgpu
backend nothing will change -RenderResourceContext::is_ready
returnstrue
(as wgpu device is available instantly after creation of WgpuPlugin), so nothing will be deferred.Related issue: #88 (BTW should I create separate issue for such PR's or it is not necessary?)