A common pattern in systems is:
void OnCreate()
{
RequireForUpdate<MySingleton>()
}
void Update()
{
GetSingleton<MySingleton>()
}
It would be really nice if GetSingleton could just automatically add the RequireForUpdate query in the generated code so we don’t need to add these pretty redundant checks.
Most people understand the singleton pattern and the pattern doesn’t usually involve checking for existence outside of the singleton implementation.
I would prefer this is not the case. I know my singletons will always exist so I have no RequireForUpdates as it’s just an unnecessary check every frame.
I knew there would be a tertle lurking around here with an advanced user perspective XD
I did think about this a little…
My immediate thought on the “know that they wont be null” approach is that it’s too fragile.
There is no direct way to see that you have handled the “null check”, so to speak, just seems a little risky to develop that way, especially in a team. Anyway to support that approach the [RequireMatchingQueriesForUpdateAttribute] could be extended to include some flags to give it options for singletons. Really it’s only singletons that are the problem, other queries are usually collected into a container then looped over so there is no risk of throwing an exception in those cases.
That being said, I do really believe that the automatic behaviour should be the default behaviour.
If you can guarantee (via other means) that your GetSingleton calls will not throw an error then you should be the one adding an annotation to override the automatic behaviour and also leaving comment on how you can make that guarantee.
My settings loader stops the world from ticking until all singletons are setup to very specifically avoid the problem of having dependencies for them.
I just decided to solve the problem by making it not a problem anymore rather than having 100s of extra dependency checks, because yeah like you i disliked having to add RequireForUpdate.
-edit-
i guess another issue would be this idea seems to go against the 1.0 change of systems being [AlwaysUpdate] by default and avoiding hidden behaviour of them not updating.
That’s a good point.
An argument can be made that singletons are a slightly different case to empty queries.
The user always expects a singleton from GetSingleton to exist however an empty query is a valid case just like an empty loop, so the user might not expect it.
But I do totally get it, its probably only a good idea if one could tell if a GetSingleton call will actually be invoked, it might be conditional and so not be required.
So perhaps a better solution is to opt in e.g. [RequireAllSingletonsForUpdate] attribute or similar.
Ultimately its a pretty small thing, just very boilerplate-y and would be nice to not have to deal with it in 1.0
An example of where force adding the dependency would break is this code and therefore has the same confusing issue of not updating.
private void OnUpdate()
{
Path1();
Path2();
}
private void Path1()
{
if (query1.IsEmptyIgnoreFilter)
return;
dependency = new Job { data = GetSingleton<MySettings>}.Schedule(query1, dependency);
}
private void Path2()
{
if (query2.IsEmptyIgnoreFilter)
return;
dependency = new Job { data = GetSingleton<MySettings2>}.Schedule(query2, dependency);
}
I don’t mind the idea of [RequireAllSingletonsForUpdate] but you could probably simplify it even to just [RequireAllForUpdate] for 95% of use cases as well other useful cases.
To be honest, updating systems based on queries should be default.
Using always run systems as default is pretty much promoting bad practices, and rather a step backwards.
Plus, it hurts performance. Plus, extra LOC to achieve same behaviour.
If user does not want to read manual, then I guess that’s a user problem, or a documentation problem.
Changing API is a bad solution for the documentation problem.
There was literally like 2 (two) usages of [AlwaysUpdateSystem] from domain code from recent 5 projects.
Now, I’d need to manually upgrade hundreds of systems to achieve same results, which would result in hundreds of LOC minimum. x4 if counting all RequireForUpdate on average.
Not to mention [RequireMatchingQueriesForUpdate] is too long and doesn’t exist for some reason in exp-8.
I personally prefer the new way it is implemented.
It is more explicit, less confusing and the additional lines needed are not really bothering me (Burstcompile everywhere is more cumbersome to me).
If we have an opt in behavior with one single attribute to create the code-gen that would be nice (as @tertle mentioned).
I disagree with this sentiment.
The most important part of API design is eliminating ambiguity. Affordances to existing patterns are very important, otherwise we are relearning how everything works every time.
I think the change to always update systems is especially great for new users, lowering the already very high barrier to entry.
They introduced ISystem everywhere to minimise system overhead anyway.
I doubt adding the [RequireMatchingQueriesForUpdate] really boosts performance that much, you might be saving 100us total over 1000 systems.
I can see [RequireMatchingQueriesForUpdate] in exp-8, think you might be mistaken on that one?
This argument is pretty much equivalent to arguing that safety checks should be off by default, because writing code that does bounds and null checking everywhere is bad practice for performance.
It is one line of code to restore the old behavior for any given system (and the attribute does exist). Having easy opt-in performance is much better than automatically performing the optimization based on criteria that is not always obvious to the user (auto-generated EntityQueries). And no one should have to read through every line of code in the system to figure out why the system isn’t running in the first place by default.
One line per system is not that bad at all. Heck, you can put it in a template and never worry about new code again. And if you are complaining about API compatibility breakage in an experimental release, well, I don’t know what to tell you. For most of us, pretty much everything was broken. 400 changes that can be solved with a regex is nothing.
I’m with most here in that I like the new design better too. Any design decision that moves away from “magic behavior” is welcome, especially when it is so easy to get back the performance once you have evaluated that it is safe to do so.
SystemBase still exists. Managed systems not going anywhere.
No checks == better. Its not about system overhead, its about not executing custom user domain logic at all.
As for the attribute. Yeah, it wasn’t detected by auto-completion for some reason.
Manully typing whole thing does the trick. Weird.
I would like to talk about the GetSingleton QoL issue at System windows at 1.0.0-exp.8. It’s quite funny that when u write GetSingleton at system without RequireForUpdate() at OnCreate(), then u still see it MySingleton query with read & write at System windows. But in fact the MySingleton is not there yet and u get error spamming. I think this System windows needs to improve this so it won’t confuse people that maybe just don’t display MySingleton query.
Actually until now I still not really understand wat’s the big difference in 1.0-exp.8 compares with 0.51. What I know it just make all systems to always update by default to improve performance but u still can use RequireForUpdate<>() at OnCreate(). Then what is the purpose of [RequireMatchingQueriesForUpdate] tag? Looks like redundant to me.