Skip to content
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

Less permissive component conflict allowance in system queries #1320

Closed
aevyrie opened this issue Jan 26, 2021 · 1 comment
Closed

Less permissive component conflict allowance in system queries #1320

aevyrie opened this issue Jan 26, 2021 · 1 comment
Labels
A-Core Common functionality for all bevy apps A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible

Comments

@aevyrie
Copy link
Member

aevyrie commented Jan 26, 2021

What problem does this solve or what need does it fill?

This is a follow-on tracking issue from #1316. In short:

There exist certain sets of queries within a single system that only work some of the time. This happens because they are allowed to run in ambiguous component access scenarios, triggering a panic at runtime only when there is an actual attempt to access conflicting components within an archetype (e.g. accessing a component while it is mutably borrowed). This permissiveness - allowing potentially conflicting queries to exist in a system - makes systems marginally easier to write, while creating "invisible" runtime errors. In addition, this permissiveness is especially dangerous to beginners who can get in the habit of creating systems with conflicting queries, without understanding why it's problematic.

The suggested changes would make Bevy less permissive of queries with component conflicts. This does not, to my knowledge, remove any functionality. Even if the most restrictive suggestion was implemented, systems that currently take advantage of this permissiveness can be fixed by wrapping conflicting queries in a QuerySet.

What solution would you like?

In order of preference:

  1. Prevent the issue entirely through some architectural change which causes an error at compile time.
  2. Error out at runtime, informing the user of the potential conflict that needs to be resolved.
  3. Warn the user when this scenario is detected, but continue to allow execution at the risk of a panic.

While (1) would require the most work and may not even be possible, users' IDEs would ideally inform them that their query is invalid while they're writing their code. This would be the least painful way to fully enforce the query component restrictions.

(2) would delay this error to runtime, which could cause some frustration until the user has learned why it's happening. Fortunately, I think the query restriction is relatively simple to teach and understand (it mirrors mutable access fundamentals in rust). I think this is the most realistic solution in the short term, and it fully addresses the problem by preventing surprise panics entirely.

I favor (3) the least because it is most likely to result in users ignoring the warnings until systems begin to mysteriously panic - at which point it might become a nightmare to debug.

What alternative(s) have you considered?

  • Provide a more helpful panic message when this case is detected, with a suggested solution. Leave general behavior unchanged.
  • Do nothing. Stick it to the man. You can't tell me what to do, you're not my dad. I'm gonna access mutably borrowed components whether you like it or not!
@karroffel karroffel added A-Core Common functionality for all bevy apps A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Jan 30, 2021
@MDeiml
Copy link
Contributor

MDeiml commented Feb 1, 2021

(1) should definitely be possible, but involves quite a lot of type level programming. It seems there has been a concious desicion not to use to much type magic, I guess because of the increase in compile time and code complexity that normally comes with it. Nevertheless I would give it a try if it's ok with everyone.

EDIT: Trying to come up with a prototype I noticed that detecting if two queries can apply to the same archetype is basically a SAT-Problem which means it can't be solved in polynomial time. This is not a huge problem since the number of variables (component types) is very small most of the time. But to detect which system truly can have conflicting queries would require implementing a simple SAT solver.

The alternative would be to use some heuristic, like treating Or<(A, ...)> as (A, ...) but this would of course mean that some valid systems would be flagged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants