Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Plugin configurability: apps own scheduling logic #33
Plugin configurability: apps own scheduling logic #33
Changes from 3 commits
c3ce1d2
4155fd6
2cebb68
b6f1776
d851b0c
c1cc3d7
e27e5a5
c4dc232
a0e52fb
587714a
dd80e25
f057f21
3adc3ac
5d83988
a9cdf54
5b4822e
71a8637
d0bb5bc
47d3b11
c355609
1c8ece0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Allowing users to add new RunCriteria to a SystemSet defined in a plugin could invalidate assumptions made by the plugin author, especially when before/after constraints dont care if a system actually runs. This could break things in hard-to-identify ways, which concerns me.
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.
This is actually one of the main reasons why I like the system sets + can only configure based on a shared label design: plugin authors could carefully control the labels they're exporting to ensure that if A doesn't run, then B also doesn't run.
However, I think this is too implicit for my taste. Instead, I think that there's an important distinction there that we're missing with the way that the current
.before
works.If A is before B, and A is skipped, what should B do? The current behaviour is for B to proceed. But that's not always correct: in some cases B should be skipped if A is skipped.
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.
Yeah I agree that there are cases where "run B only if A ran" would be a useful tool and could help prevent certain instances of this problem. Haha we're starting to have quite the menagerie of order types: "the current soft-orders", "hard orders (side effects applied", and "only-if orders". We might need to come up with new names for these concepts / sort out the right way to express "soft order", "soft order with side effects", "only-if order", and "only-if order with side effects".
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'd like this RFC to explicitly address how Plugins will handle "extending" existing resources. Ex:
register_type::<T>()
to add T to theTypeRegistry
resourceinit_asset_loader::<T>()
to add a new asset loader to theAssetServer
resourceThis requires solving the "order of operations" problem. When will Plugins be initialized? How will we ensure that
TypeRegistry
exists beforeregister_type
is called?I can think of two options:
Plugins have dependencies
Plugins have dependencies, either by implicitly defining them via something like registration order or explicitly defining them via something like labels (or Plugins providing a list of TypeId dependencies). This is basically accepting that Plugins can have arbitrary side effects. At this point, we might as well give direct access to World. Plugins have more freedom to interact with the app and implementing things like
register_type
is both simple and efficient (no need to schedule any extra systems, define dependencies between them, etc).In some ways, this is what we have today. But we could still constrain things as defined in this RFC / add the additional system set configurability. And we could still have "deferred plugin init" if theres a good reason for it.
Plugins do not have dependencies
Plugins are "just" order-independent descriptions of systems to add. Only Systems can have orders. Some initialization things depend on specific resources, so resources must be added via Systems. Something like
plugin.init_asset_loader::<T>()
would internally expand to something like:Some things to note:
plugin.add_resource(MyResource { foo, bar })
orplugin.add_asset_loader(MyLoader::new())
.Lets take a look at why (2) isn't possible:
First, this would have the benefit of being able to schedule init stuff in parallel. But honestly this type of resource init is generally cheap. I doubt that the parallelism wins will outweigh the overhead of System scheduling.
However we can't do this right now: we would need a RunOnce system equivalent to accomplish this, which would be a pretty significant change, both to the System trait and the Scheduler implementation.
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.
Could we have some kind of plugin resource (name bikeshedding necessary) concept. Plugins would then specify some data and the plugin resource will be called when plugins are added and will handle this data appropriately? For example in case of the TypeRegistry plugin resource a plugin can provide type registrations, while for the AssetServer plugin resource a plugin can provide asset loaders. This would possibly also allow handling removal of plugins using an extra callback in the future.
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 don't fully follow this. Could you walk through the "order of operations" of how the AssetPlugin registers the AssetServer and some CustomPlugin registers a custom AssetLoader? How would the AssetLoader be represented in the CustomPlugin? When / how would it be registered with the AssetServer resource?
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.
AssetPlugin would register AssetServer as "plugin resource" using something like
self.add_plugin_resource(AssetServer::default())
. CustomPlugin schedules an add event for CustomAssetLoader for the AssetServer "plugin resource" using something likeself.plugin_resource_add_part::<AssetServer>(CustomAssetLoader::default())
. The order between the these two registrations doesn't matter. TheAssetServer
could then either be notified about the add ofCustomAssetLoader
after all plugins are registered or as soon as bothAssetServer
andCustomAssetLoader
are registered. It doesn't really matter which one is chosen. Notably in either case if theAssetServer
is not yet registered by the timeCustomPlugin
tries to registerCustomAssetLoader
, the actual registration is delayed until afterAssetServer
is registered.AssetServer
has to implement a trait with a method that is called every time something is registered with it.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.
Yup that seems like it would work. This seems similar to the "Plugins don't have dependencies" solution (due to the need to ensure that AssetServer is registered prior to applying "resource parts"), but with more complicated userspace (due to the addition of "resource parts" as a concept). It means that in addition to the "normal" apis exposed on a resource, developers also need to think about the "resource plugin api" and expose the relevant bits. Feels like a bit too much complexity. If we can detect the
resource_part -> resource
dependency, then we can just as easily provide the custom asset loader registration with direct (but deferred) access to the AssetServer resource without the need for a new public api layer.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 guess so. It doesn't help with removing plugins again though for the editor or runtime loadable mods. It also doesn't allow inspecting whatever the plugin injected from the editor as easily.