Skip to content

Deprecate panicking .resource methods #18084

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

Closed

Conversation

alice-i-cecile
Copy link
Member

Objective

This PR is a sibling to #18082, and almost all of the motivation there applies here. These methods are much more commonly used in commands, tests and exclusive systems though, and deserve their own judgement call.

Ultimately part of #14275.

Solution

  1. Deprecate all of the panicking resource methods I can find.

I'm less confident about this PR, and there's 796 warnings in my IDE, so I want to get consensus before burning time on migrating all of these.

TODO:

  • fix deprecation warnings
  • add docs?
  • [] make World::resource and friends return a Result for nicer ? ergonomics?

Future Work

Move the non-panicking versions to the better names during the 0.17 cycle, just like in #18082.

Migration Guide

World::resource and all related methods are now deprecated. Use World::get_resource and so on and handle the returned Option.

@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 X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required labels Feb 28, 2025
@alice-i-cecile alice-i-cecile requested a review from cart February 28, 2025 00:02
@alice-i-cecile alice-i-cecile marked this pull request as draft February 28, 2025 00:02
@alice-i-cecile
Copy link
Member Author

We could choose to use a lint for these methods and keep them around (see TheBevyFlock/bevy_cli#269) and keep them around, but I prefer the solution here because:

  1. Our defaults should be the right choice for finished games.
  2. Less API = More Good.
  3. Relying on external unshipped tooling sucks.

@JMS55
Copy link
Contributor

JMS55 commented Feb 28, 2025

This is going to make a lot of rendering code worse :/ (check how often .resource::< is called). Why not keep both methods around? If we want to make the engine itself more resilient, then I think targeted audits to find places we shouldn't be panicking is a better idea.

@alice-i-cecile
Copy link
Member Author

Yeah, I think that's reasonable. Given that we're not running into many panics here (and Bevy already panics when you're missing a resource in a normal system) I think this is net-negative. Closing out.

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-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants