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

Replace FromWorld blanket implementation with derive macro #4265

Closed
Shatur opened this issue Mar 20, 2022 · 9 comments
Closed

Replace FromWorld blanket implementation with derive macro #4265

Shatur opened this issue Mar 20, 2022 · 9 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@Shatur
Copy link
Contributor

Shatur commented Mar 20, 2022

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

Currently resources can have either FromWorld or Default trait implementation because of blanked FromWorld implementation. This causes inconveniences with other crates integration. For example, confy crate requires configuration structure to have Default trait, so I can't create a convenient FromWorld for it.

What solution would you like?

I would suggest to switch to explicit derive macro.

@Shatur Shatur added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 20, 2022
@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 and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 20, 2022
@cart
Copy link
Member

cart commented Mar 21, 2022

Implicit FromWorld impls for Default impls means that we can use both traits in things like app.init_resource::<T>(). Requiring a derive seems like it would be a pretty massive ergonomics (and conceptual) loss, as it would mean we can no longer rely on Default impls in these cases. Anything that currently uses a default impl would either also need a manual FromWorld impl or would need to replace Default with FromWorld. I'd want to see how this affects real-world use cases (and the official bevy examples) before committing to it.

Right now thinking about FromWorld as "Default but for things that need World access" seems to be working reasonably well. Use Default generally, unless you need world access. Then implement FromWorld instead. If you can implement Default, by definition you don't need FromWorld. What is the specific use case you're trying to enable?

@alice-i-cecile
Copy link
Member

As a more scoped workaround:

  1. Remove the blanket impl of FromWorld for T: Default.
  2. Modify the init_resource methods to only take T: Default.
  3. Create a new init_resource_from_world method that takes T: FromWorld.

I would also like to hear a bit more about the use cases before pursuing that though.

@Shatur
Copy link
Contributor Author

Shatur commented Mar 21, 2022

Thanks for the replies!

Right now thinking about FromWorld as "Default but for things that need World access" seems to be working reasonably well. Use Default generally, unless you need world access. Then implement FromWorld instead. If you can implement Default, by definition you don't need FromWorld. What is the specific use case you're trying to enable?

Sometimes I need to have Default trait and a different FromWorld implementation. For example, here I have a struct that used as a resource and read from command line. I have defaults() function that I use for default initialization and FromWorld implementation that checks if the command line argument was provided.
Right now my defaults() function is just a function that doesn't implement Default trait because of the mentioned limitation.
Is this a valid use-case?

@Shatur Shatur changed the title Replace FromWorld blanked implementation with derive macro Replace FromWorld blanket implementation with derive macro Mar 21, 2022
@alice-i-cecile
Copy link
Member

The workaround doesn't seem too bad: I'm reluctant to degrade the common-case UX for this. Would my solution with split methods work for you?

@Shatur
Copy link
Contributor Author

Shatur commented Mar 21, 2022

Would my solution with split methods work for you?

Yes, it's much better then I suggested. And I like its explicitness: init_resource - uses Default trait, init_resource_from_world - obviously uses FromWorld.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 21, 2022

Agreed: I think its explicitness is quite good, and from a UX perspective I would say that's probably a wash when weighed against adding yet another set of methods.

I think overall I'd personally be happy enough to see a PR with that change (although in the long-run this feels like yet another "plugins should be structured" problem).

@cart
Copy link
Member

cart commented Mar 21, 2022

Idk if I want to add another dimension to the "resource setup" category of things:

  • init_resource
  • init_non_send_resource
  • init_resource_from_world
  • init_non_send_resource_from_world
  • insert_resource
  • insert_non_send_resource

(with impls on both World and App)

Thats a pretty big set of apis for users to sort through. And it means that if we need to add another variant in the future, this set of apis will "balloon out of control".

@alice-i-cecile
Copy link
Member

Yep, same problem as #1978. Unfortunately I don't really see a good way to apply the pattern used there here :/

@Shatur
Copy link
Contributor Author

Shatur commented Mar 22, 2022

Got it, probably there is no good solution. I will move my resource initialization into "plugin" as a workaround then.

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

No branches or pull requests

3 participants