-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Improve error message for EntityNotSpawnedError #22444
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
Conversation
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
ElliottjPierce
left a comment
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.
First of all, as far as I'm concerned, this is an aesthetic change, and I'll be the first to admit that my error messages may not be friendly to new users. Probably just been using these terms to understand it myself, and sometimes I forget that not everyone (especially those not obsessed with the internals of entities) thinks in the same terms.
So, I'm happy with anything here. Usually you guys have better naming ideas than me anyway.
With that said, here's some feedback: The EntityNotSpawnedError::Invalid rarely means the entity was not spawned. For that to happen, a made up Entity would need to be used which has not yet been spawned. The error in this pr's example, is not that the entity had not been spawned, but that it had been despawned since.
We should also consider helping out EntityNotSpawnedError::ValidButNotSpawned. This can happen because of entity generation wrapping, but it is more commonly caused when an entity has been allocated/reserved but not fully initialized yet.
So, my suggestion for the error prefix might be more like:
"Entity despawned: " for Invalid and "Entity not yet spawned: " for ValidButNotSpawned. But neither prefix covers all the possible causes–just the most common.
Either way, more docs here would certainly be good. Maybe this is something for the bevy error people to look at? I haven't been keeping up with that, but IIRC, there was a team working on adding links and docs around common errors.
|
awesome thanks for that context, here's the updated version |
ElliottjPierce
left a comment
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 looks good to me!
|
PR targets the release branch, so the visual diff comment is because it's diffing from "main's version" to the release version and not because there are real changes. |
|
This is targeting the wrong branch: it needs to target main, then get cherrypicked on. I tried swapping this with the Github UI, but the diff got messed up. Take a look, and remake the PR if it starts to be a headache. If this is merged in time we'll ship it with 0.18 :) |
|
closing in favor of same pr on main branch #22467 |
The new
0.18entity errors feel a bit 'inside baseball' for end-users, for instance interacting with a despawned entity makes no mention of the entity not being spawned:Ideally I'd like to implement proper rust-style errors with a short paragraph detailing common causes and links to docs but thats a bigger discussion.
At least for
0.18i think this very common error needs a prefixEntity Not Spawned:The entity with ID 27v0 is invalid; its index now has generation 1.Entity Not Spawned: The entity with ID 27v0 is invalid; its index now has generation 1.