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

[Merged by Bors] - Expose transform propagate systems #7145

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Expose transform propagate systems #7145

wants to merge 4 commits into from

Conversation

lewiszlw
Copy link
Member

Objective

I don't know if it's reasonable that making propagate_transforms public. I also created an issue to bevy_rapier dimforge/bevy_rapier#307 to see how offical team will solve this issue.

Solution

  • make propagate_transforms system public.

@james7132
Copy link
Member

You need to use both sync_simple_transforms and propagate_transforns for a full propagation. I don't like the idea of exposing them individually, and without the system labels. Could we make the plugin non-unique and allow it to be added with different system labels?

@alice-i-cecile
Copy link
Member

Won't work unfortunately, since you can't yet control when plugin systems run.

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales labels Jan 10, 2023
@lewiszlw
Copy link
Member Author

Could we make the plugin non-unique and allow it to be added with different system labels?

I'm trying to store stages in plugin like this,

pub struct TransformPlugin {
    stages: Vec<Box<dyn StageLabel>>
}
impl Default for TransformPlugin {
    fn default() -> Self {
        Self { stages: vec![Box::new(StartupStage::PostStartup), Box::new(CoreStage::PostUpdate)] }
    }
}

impl Plugin for TransformPlugin {
    fn build(&self, app: &mut App) {
        ...
        for stage in self.stages {
            app.add_system_to_stage(stage, systems::propagate_transforms.label(TransformSystem::TransformPropagate))
        }
    }

    fn is_unique(&self) -> bool {
        false
    }
}

There are two problems

  1. Don't satisfy Plugin trait bound
  2. Can't pass Box<dyn StageLabel> to add_system_to_stage method as param is impl StageLabel

What's your thought to allow plugin to be added with different system labels? @james7132

@mockersf
Copy link
Member

You need to use both sync_simple_transforms and propagate_transforns for a full propagation. I don't like the idea of exposing them individually, and without the system labels.

Could we expose a single system that would run both? Not used in Bevy, just for external consumption for the case when someone wants to run global transform propagation when they want

@james7132
Copy link
Member

Could we expose a single system that would run both? Not used in Bevy, just for external consumption for the case when someone wants to run global transform propagation when they want

We could just have a function that returns a SystemSet. That'd allow users to do what they want with it (add it to their own stages, label them however they wish, etc).

@mockersf
Copy link
Member

Could we expose a single system that would run both? Not used in Bevy, just for external consumption for the case when someone wants to run global transform propagation when they want

We could just have a function that returns a SystemSet. That'd allow users to do what they want with it (add it to their own stages, label them however they wish, etc).

Sounds good! @lewiszlw could you add this function as public, and mention it in the docs or comments of propagate_transforms and sync_simple_transforms, and the docs of GlobalTransform (https://dev-docs.bevyengine.org/bevy/prelude/struct.GlobalTransform.html)?

@lewiszlw
Copy link
Member Author

Sounds good! @lewiszlw could you add this function as public, and mention it in the docs or comments of propagate_transforms and sync_simple_transforms, and the docs of GlobalTransform (https://dev-docs.bevyengine.org/bevy/prelude/struct.GlobalTransform.html)?

Sure. I'm not very good at English, feel free to correct doc.

@lewiszlw lewiszlw changed the title Expose propagate_transforms system Expose transform propagate systems Jan 11, 2023
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.

Small corrections for the docs, then this looks good to me.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 11, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 11, 2023
# Objective

- I tried to create a fork of bevy_rapier to track latest bevy main branch. But bevy_rapier depends on bevy internal `propagate_transforms` system (see https://github.com/dimforge/bevy_rapier/blob/master/src/plugin/plugin.rs#L64).
- `propagate_transforms` system was changed to private in #4775.

I don't know if it's reasonable that making `propagate_transforms` public. I also created an issue to bevy_rapier dimforge/bevy_rapier#307 to see how offical team will solve this issue.

## Solution

- make `propagate_transforms` system public.
@bors bors bot changed the title Expose transform propagate systems [Merged by Bors] - Expose transform propagate systems Jan 11, 2023
@bors bors bot closed this Jan 11, 2023
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

- I tried to create a fork of bevy_rapier to track latest bevy main branch. But bevy_rapier depends on bevy internal `propagate_transforms` system (see https://github.com/dimforge/bevy_rapier/blob/master/src/plugin/plugin.rs#L64).
- `propagate_transforms` system was changed to private in bevyengine#4775.

I don't know if it's reasonable that making `propagate_transforms` public. I also created an issue to bevy_rapier dimforge/bevy_rapier#307 to see how offical team will solve this issue.

## Solution

- make `propagate_transforms` system public.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- I tried to create a fork of bevy_rapier to track latest bevy main branch. But bevy_rapier depends on bevy internal `propagate_transforms` system (see https://github.com/dimforge/bevy_rapier/blob/master/src/plugin/plugin.rs#L64).
- `propagate_transforms` system was changed to private in bevyengine#4775.

I don't know if it's reasonable that making `propagate_transforms` public. I also created an issue to bevy_rapier dimforge/bevy_rapier#307 to see how offical team will solve this issue.

## Solution

- make `propagate_transforms` system public.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Usability A simple 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.

4 participants