Skip to content

Make Plugin::{build, setup} take &mut self instead of &self#8101

Closed
geieredgar wants to merge 1 commit intobevyengine:mainfrom
geieredgar:mut-plugins
Closed

Make Plugin::{build, setup} take &mut self instead of &self#8101
geieredgar wants to merge 1 commit intobevyengine:mainfrom
geieredgar:mut-plugins

Conversation

@geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Mar 15, 2023

Objective

Solution

  • Change Plugin::build and Plugin::setup to take &mut self instead of &self.

Changelog

Changed

  • Plugin::build and Plugin::setup now take &mut self instead of &self.

Migration Guide

  • Implementers of Plugin need to change their build and setup implementations to take &mut self instead of &self.

@geieredgar geieredgar mentioned this pull request Mar 15, 2023
Closed
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins labels Mar 16, 2023
alice-i-cecile
alice-i-cecile approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'm on board with this: this isn't the first time I've seen users trip over this.

Code change itself is nearly trivial.

@hymm
Copy link
Contributor

hymm commented Mar 17, 2023

note this potentially goes would go against cart's comment here that plugin build should be stateless.

#4212 (comment)

Something else worth calling out: consuming self would mean that the Plugin's init isn't a repeatable process, which might come in handy for things like editors, hot reloading, etc.

I think we should be moving in the direction of "plugins are completely stateless by default and configured with an instance of a specific type (ideally with a default impl)"

@alice-i-cecile alice-i-cecile added the X-Needs-SME This type of work requires an SME to approve it. label Mar 17, 2023
@geieredgar geieredgar mentioned this pull request Mar 17, 2023
Closed
@cart
Copy link
Member

cart commented Mar 21, 2023

Yeah I think we should hold this line for now until we have the editor situation sorted out (and maybe also hot code reloading). The wins we get here don't outweigh the cost of trying to put this genie back in the bottle across the ecosystem. Anything that requires "state consumption" can use ECS resources for now.

We can reopen (or likely re-implement) if after we've sorted everything out we decide we can do this.

@cart cart closed this Mar 21, 2023
@eerii eerii mentioned this pull request Apr 13, 2023
Closed
Estus-Dev pushed a commit to Estus-Dev/bevy that referenced this pull request Jul 10, 2023
@eerii
Support for user defined log formatting (bevyengine#8374)
82f8e93 
# Objective

Adopted bevyengine#3778.

This pr allows to configure how the bevy log is formatted. The default
bevy log can be a bit verbose for some projects, specially the
timestamp. The objective is to allow developers to select the format
that best suits them.

```
Example of original bevy log:
2023-04-13T14:19:53.674653Z INFO bevy_render::renderer: [important text!]
```

## Solution

Using `tracing_subscriber::fmt::Subscriber`, adds the attribute `layer`
to `LogPlugin`. This allows to pass a `tracing_subscriber::fmt::Layer`
when creating the plugin and modifying the output.

For example this

```rust
App::new()
    .add_plugins(DefaultPlugins.set(bevy::log::LogPlugin {
        layer: Box::new(|| Box::new(bevy::log::tracing_subscriber::fmt::layer().pretty().without_time()))
        ..default()
    }))
    .run()
```

prints

```
INFO bevy_render::renderer: [important text!]
```

The main idea comes from @SonicZentropy and their pull request bevyengine#3778,
but it was stale since may. This has all the changes and a fix since now
`LogSettings` are included in the plugin and not in a resource.

Since plugin builders are stateless and not mutable (bevyengine#8101), the old
solution of using an `Option<Box<Layer>>` no longer works. One solution
to fix this is to use a `Box<Fn() -> Box<Layer>>`. I am unaware of
another more idiomatic fix, so if you have something better please
revise it. Should plugins builders be mutable in the future, this can be
changed back to the original approach.

---

## Changelog

- Adds field `layer` to `LogPlugin` that takes a function returning a
`tracing_subscriber` custom layer.
- Reexports `tracing_subscriber` from log so it can be used to configure
the plugin.
- Modifies the `logs.rs` and `system_piping.rs` examples to include the
new configuration.

## Migration Guide

No breaking changes
Estus-Dev pushed a commit to Estus-Dev/bevy that referenced this pull request Jul 10, 2023
@eerii
Support for user defined log formatting (bevyengine#8374)
9920b64 
# Objective

Adopted bevyengine#3778.

This pr allows to configure how the bevy log is formatted. The default
bevy log can be a bit verbose for some projects, specially the
timestamp. The objective is to allow developers to select the format
that best suits them.

```
Example of original bevy log:
2023-04-13T14:19:53.674653Z INFO bevy_render::renderer: [important text!]
```

## Solution

Using `tracing_subscriber::fmt::Subscriber`, adds the attribute `layer`
to `LogPlugin`. This allows to pass a `tracing_subscriber::fmt::Layer`
when creating the plugin and modifying the output.

For example this

```rust
App::new()
    .add_plugins(DefaultPlugins.set(bevy::log::LogPlugin {
        layer: Box::new(|| Box::new(bevy::log::tracing_subscriber::fmt::layer().pretty().without_time()))
        ..default()
    }))
    .run()
```

prints

```
INFO bevy_render::renderer: [important text!]
```

The main idea comes from @SonicZentropy and their pull request bevyengine#3778,
but it was stale since may. This has all the changes and a fix since now
`LogSettings` are included in the plugin and not in a resource.

Since plugin builders are stateless and not mutable (bevyengine#8101), the old
solution of using an `Option<Box<Layer>>` no longer works. One solution
to fix this is to use a `Box<Fn() -> Box<Layer>>`. I am unaware of
another more idiomatic fix, so if you have something better please
revise it. Should plugins builders be mutable in the future, this can be
changed back to the original approach.

---

## Changelog

- Adds field `layer` to `LogPlugin` that takes a function returning a
`tracing_subscriber` custom layer.
- Reexports `tracing_subscriber` from log so it can be used to configure
the plugin.
- Modifies the `logs.rs` and `system_piping.rs` examples to include the
new configuration.

## Migration Guide

No breaking changes
@alice-i-cecile alice-i-cecile mentioned this pull request Jan 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees

No one assigned

Labels

A-App Bevy apps and plugins C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Needs-SME This type of work requires an SME to approve it.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@geieredgar @hymm @cart @alice-i-cecile