Skip to content

Conversation

@MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Jul 9, 2024

Objective

Solution

  • As described in the issue

Testing

  • Ran the example from the issue
    image

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 9, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jul 9, 2024
Copy link
Contributor

@benfrankel benfrankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small PR, one nit.

@mockersf
Copy link
Member

mockersf commented Jul 10, 2024

Could you check the stack trace, and how easy it is to find where the issue comes from? Could track_caller help?

Also, what does it mean to "install" a plugin? I prefer the error message from #14160

@mockersf
Copy link
Member

mockersf commented Jul 10, 2024

I would also prefer to not have a hard dependency on the plugin, but on what it's adding. If someone decides to rewrite the plugin changing a few things but still setting up all the machinery, it should still work. #14160 is better for that

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Jul 12, 2024

I would also prefer to not have a hard dependency on the plugin, but on what it's adding. If someone decides to rewrite the plugin changing a few things but still setting up all the machinery, it should still work. #14160 is better for that

Agree to disagree, most of the machinery is in the app methods. StatePlugin only adds StateTransitions to startup and main schedules and register the core state set. The only valid use case for swapping just the plugin would be to remove StateTransitions from startup, which doesn't sound bad, but can cause 3rd party crates to work incorrectly if they relied on this.

So replacing the machinery would require replacing both, the plugin and the methods. And that's still at the risk of breaking 3rd party crates

@MiniaczQ
Copy link
Contributor Author

Could you check the stack trace, and how easy it is to find where the issue comes from? Could track_caller help?

I think it's plenty readable as is:
image

With [track_caller]:
image

@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 12, 2024
@alice-i-cecile
Copy link
Member

I would also prefer to not have a hard dependency on the plugin, but on what it's adding. If someone decides to rewrite the plugin changing a few things but still setting up all the machinery, it should still work. #14160 is better for that

In addition to the feedback above; this will also be much harder to maintain. I don't think we have a clear / stable enough set of invariants to make this feasible right now.

Copy link
Member

@mnmaita mnmaita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still relevant now that #14160 was merged? @alice-i-cecile

@alice-i-cecile
Copy link
Member

I'd like to avoid panics unless absolutely essential, and I think that #14160 is an adequate solution to this problem. Closing.

@MiniaczQ MiniaczQ deleted the panic-state-plugin branch September 7, 2024 19:14
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

State API should panic when missing StatePlugin Improve error message when StatesPlugin is missing

6 participants