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

If we try to write to an entity that doesn't exist, log it rather than completely crash the game. #2219

Closed
wants to merge 1 commit into from

Conversation

ndarilek
Copy link
Contributor

Admittedly this should probably be fixed in the bowels of the ECS where I'm not smart enough to wander, but I've hit this again and again to the point where it's holding back feature development. Every single time I've hit this crash, it's been when writing to an entity that I removed--enemy I destroyed, an entity that no longer exists because I reset the game, etc. Who cares if the ECS tries to write hit points to a monster that was killed and removed entirely from the game? I don't--certainly not enough to bring down the entire world. :)

I don't know if this should log at a higher level--debug, maybe?

I hesitate to call this a fix for #1743 because again, there's probably a proper fix. But can we please stop crashing? Aside from occasional log messages, this works well for me and I've had no crashes since.

Sorry if I seem cranky--it'd just be nice to not bring things to a screeching halt if we can at all avoid it. :) Thanks.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events help wanted C-Usability A targeted quality-of-life change that makes Bevy easier to use labels May 19, 2021
@NathanSWard
Copy link
Contributor

NathanSWard commented May 19, 2021

I don't know if I completely agree with this judgement.
If you are trying to write to an entity, and that entity is invalid, that should not just silently succeed (minus the warn!), it should crash.

For example if you had a game that was in production, and you were trying to write to an invalid entity, I don't know if I would want to to succeed and meerly log a message, as now my game might be in an invalid state and have unpredictable behavior. (Or crash elsewhere and make the bug harder to track down).

If we wanted a solution to this, my proposal would be to extend the API on Commands to support a safe path where if the entity does not exist, log the warning and just continue on.

E.g. something like

fn system(mut commands: Commands) {
    let invalid_entity = //...

    // crashes the app because the entity is invalid
    commands.entity(invalid_entity).insert(MyComponent);

    // does not crash the app, just silently succeeds (and logs if logging is enabled)
    commands.entity_safe(invalid_entity).insert(MyComponent);
}

@mockersf
Copy link
Member

If we wanted a solution to this, my proposal would be to extend the API on Commands to support a safe path where if the entity does not exist, log the warning and just continue on.

Seems a good idea, but not safe here. It's a word heavy with meaning in Rust, plus you would be calling "safe" the path that leads to "UB" of believing the entity exists when it doesn't

@NathanSWard
Copy link
Contributor

NathanSWard commented May 19, 2021

Seems a good idea, but not safe here. It's a word heavy with meaning in Rust, plus you would be calling "safe" the path that leads to "UB" of believing the entity exists when it doesn't

Yea, haha I don't want to use safe as well, that was more so to just encourage bike-shedding on what it should actually be called 😛

@ndarilek
Copy link
Contributor Author

In general I agree that this shouldn't fail in any way. However, I'm hugely lucky if I know which system caused it so I can even begin debugging the issue. And, more often than not, if I manage to identify the system, I then discover that the failure isn't at all deterministic, and no amount of reordering fixes it.

I don't think there should be two paths. I think there should be a single functional path, and until Bevy can either guarantee that path works or give me actionable advice that I can trace back to a system and fix a known problem, then it shouldn't just crash. I'm effectively blocked right now because Bevy is trying to write to entities I'm despawning when my game is reset. It's nothing wild and revolutionary, and certainly nothing that should reset the game by dumping me back to my command prompt. :)

@NathanSWard
Copy link
Contributor

NathanSWard commented May 19, 2021

However, I'm hugely lucky if I know which system caused it so I can even begin debugging the issue. And, more often than not, if I manage to identify the system, I then discover that the failure isn't at all deterministic, and no amount of reordering fixes it.

I agree there could be better messages here.
In debug mode, we could track the name of the system and print the name on an invalid entity access.
And in release we just crash.
We don't want to add the extra overhead in release mode of passing the system name through the commands struct, but in debug this is probably ok.

I don't think there should be two paths.

I agree here as well, having two paths adds extra confusion.
Well... however, one could argue that this is actually ok?
If you want two systems to run in parallel (one that conditionally despawns entities and the other that writes to them) this is not possible without a not-crashing-path.

It's nothing wild and revolutionary, and certainly nothing that should reset the game by dumping me back to my command prompt.

Yes I agree that just completely crashing the app (especially in debug is not a good user experience).
However I would argue that especially in release mode, if you are trying to write to an invalid entity, that is simply an error and the app should not just continue.

@ndarilek
Copy link
Contributor Author

ndarilek commented May 19, 2021 via email

@mockersf
Copy link
Member

While not a complete fix for your issue, I just pushed a PR that would log the system name that created a failing command (under a feature): #2222

Proper error handling from commands is something that we want to add, rather than just panicking. An issue is opened for that, see #2004

@NathanSWard
Copy link
Contributor

Anyway, I'm out of the discussion. :) Again, I'm not ragey, just
passionate and a bit frustrated. Bevy's gonna get a lot less fun if it
just randomly crashes on me in some way I can't fix, and nothing gets
done about it because we don't have a technically correct fix yet.

No worries, and thank you for bringing this issue up. <3
It's definitely something that is causing a bad UX and something we should prioritize.
I'm currently working on getting a bevy development process workflow up and running.
This will help us better prioritize our issues and needs (and for example, prioritize this issues).

@NathanSWard
Copy link
Contributor

Since we have #2222 addressing a temporary solution for this issue and we have #2004 tracking the issue at large, unless anyone has a specific reason, we can probably close this PR.

@NathanSWard
Copy link
Contributor

Closing this since we have #2004 to track the issue.
Also I have a PR that'll come up soon to hopefully address and expose a new API for Command error handling.

@NathanSWard
Copy link
Contributor

@ndarilek

Just as a follow up to this PR, we have work in #2241 for specify error handlers for Commands.
And specifically, now the default behavior is to not panic, but instead log the error via an error! message :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants