Implement SystemCondition for systems returning Result<bool, BevyError> and Result<(), BevyError> #19553
+127
−20
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.
Objective
Fixes #19403
As described in the issue, the objective is to support the use of systems returning
Result<(), BevyError>
andResult<bool, BevyError>
as run conditions. In these cases, the run condition would hold onOk(())
andOk(true)
respectively.Solution
IntoSystem<In, bool, M>
cannot be implemented for systems returningResult<(), BevyError>
andResult<bool, BevyError>
as that would conflict with their trivial implementation of the trait. That led me to add a method to the sealed traitSystemCondition
that does the conversion. In the original case of a system returningbool
, the system is returned as is. With the new types, the system is combined withmap()
to obtain abool
.By the way, I'm confused as to why
SystemCondition
has a genericIn
parameter as it is only ever used withIn = ()
as far as I can tell.Testing
I added a simple test for both type of system. That's minimal but it felt enough. I could not picture the more complicated tests passing for a run condition returning
bool
and failing for the new types.Doc
I documenting the change on the page of the trait. I had trouble wording it right but I'm not sure how to improve it. The phrasing "the condition returns
true
" which reads naturally is now technically incorrect as the new types return aResult
. However, the underlying condition system that the implementing system turns into does indeed returnbool
. But talking about the implementation details felt too much. Another possibility is to use another turn of phrase like "the condition holds" or "the condition checks out". I've left "the condition returnstrue
" in the documentation ofrun_if
and the provided methods for now.I'm perplexed about the examples. In the first one, why not implement the condition directly instead of having a system returning it? Is it from a time of Bevy where you had to implement your conditions that way? In that case maybe that should be updated. And in the second example I'm missing the point entirely. As I stated above, I've only seen conditions used in contexts where they have no input parameter. Here we create a condition with an input parameter (cannot be used by
run_if
) and we are using it withpipe()
which actually doesn't need our system to implementSystemCondition
. Both examples are also callingIntoSystem::into_system
which should not be encouraged. What am I missing?