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

Introduce mutable version of ChangedRes<> #598

Closed
ropewalker opened this issue Sep 28, 2020 · 8 comments
Closed

Introduce mutable version of ChangedRes<> #598

ropewalker opened this issue Sep 28, 2020 · 8 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@ropewalker
Copy link

Currently if I want to a) execute a system only if a certain resource has been modified and b) to modify that resource in that system, I can do something like this:

pub fn my_system(
    _: ChangedRes<MyRes>,
    mut my_resource: ResMut<MyRes>,
) {
//...
}

It would be nice to be able to use just one parameter instead, like this:

pub fn my_system(
    mut my_resource: ChangedResMut<MyRes>,
) {
//...
}
@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Sep 28, 2020
@ropewalker ropewalker changed the title Introduce mutable version of ChangeRes<> Introduce mutable version of ChangedRes<> Sep 28, 2020
@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 29, 2020

wait that first code snippet works... Isn't that totally UB by providing a &MyRes and an &mut MyRes??

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 29, 2020

use bevy::prelude::*;

fn main() {
    App::build()
        .add_default_plugins()
        .add_startup_system(startup.system())
        .add_system(ub.system())
        .run();
}

#[derive(Debug)]
pub struct MyRes(f32);

pub fn startup(mut commands: Commands) {
    commands.insert_resource(MyRes(0.));
}

pub fn ub(res_1: Res<MyRes>, mut res_2: ResMut<MyRes>) {
    dbg!(&*res_1);
    res_2.0 += 2.;
    dbg!(&*res_1);
}

Welp this prints out an ever increasing number so I'm gonna go ahead and say this is UB unless I have a fundamental misunderstanding of how resources work x]

@ropewalker
Copy link
Author

@EllenNyan I am not a big expert in smart pointers, but isn't the logic in your example the same as in the following?

use std::cell::RefCell;
use std::rc::Rc;

fn main() {
    let res_1 = Rc::new(RefCell::new(0.));
    let res_2 = Rc::clone(&res_1);
    
    println!("res_1 before mutation = {:?}", *res_1);
    println!("res_2 before mutation = {:?}", *res_2);

    *res_2.borrow_mut() += 2.;

    println!("res_1 after mutation = {:?}", *res_1);
    println!("res_2 after mutation = {:?}", *res_2);
}

At least I would intuitively expect resources to work somewhat like this.

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 29, 2020

Resources in bevy aren't using Rc<RefCell<T>>, Res<T> internally just contains an &'a T, ResMut<T> stores a *mut T internally.

For what it's worth, for each systems have the same issue.

pub fn ub_2(mut left: Mut<usize>, right: &usize) {
    dbg!(right);
    *left += 2;
    dbg!(right);
}

this will print out a constantly ascending number :)

@ropewalker
Copy link
Author

FWIW, this doesn't panic and does go infinite:

use bevy::prelude::*;

fn main() {
    App::build()
        .add_default_plugins()
        .add_startup_system(startup.system())
        .add_system(ub.system())
        .run();
}

pub struct MyRes(Vec<usize>);

pub fn startup(mut commands: Commands) {
    commands.insert_resource(MyRes(vec![0]));
}

pub fn ub(res_1: Res<MyRes>, mut res_2: ResMut<MyRes>) {
    dbg!("start of the system");
    for i in 0..res_1.0.len() {
        dbg!(res_1.0[i]);
        res_2.0.push(i);
    }
}

@alice-i-cecile
Copy link
Member

This should be solved by #1313.

@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Feb 17, 2021
@alice-i-cecile
Copy link
Member

Double-checking: @TheRawMeatball is this resolved now?

@TheRawMeatball
Copy link
Member

Yes, it is. Closing.

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-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

5 participants