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

Compress default bundles #1588

Closed

Conversation

TheRawMeatball
Copy link
Member

This uses the nested bundles from #1525 to simplify the built-in bundles.

@TheRawMeatball TheRawMeatball added the C-Code-Quality A section of code that is hard to understand or change label Mar 7, 2021
@cart
Copy link
Member

cart commented Mar 7, 2021

I'm not sure we should use bundle nesting internally for most things. They make bundle consumption more difficult / boilerplate-ey. If this pr were to also update the examples to account for bundle changes, I think we would see a significant reduction in clarity and terseness.

Relevant discussion on TransformBundle here: #1460 (comment)

We should probably start a Github Discussion about when/where to recommend and use bundle nesting.

@mtsr
Copy link
Contributor

mtsr commented Mar 11, 2021

I do think this might make it easier to compose your own bundles. Although definition is indeed much less common than usage.

A problem with the current bundles is that if you want to override or get rid of one of the components, you have to make your own bundle with all components copied from the default bundle. Take for example wanting to use PBR in a different Pass. And this becomes more of a problem for the pieces that are changing quickly (unlike, probably, Transform).

I do agree that the example (copied below) is painful. I'm not sure whether overriding only the transform is the common case, or if it might show this to be worse than it actually is? Also, might there be ways to make this case simpler somehow?

Particularly transform could be solved by something like transform_bundle: Transform::from_xyz(1.0, 0.0, 0.0).into(), although that messes up how homogenous the spawns now are and introducing From as yet another thing to be aware of sucks too.

Finally, I'm not sure whether this will offer enough improvement at this point to invest much effort in it and it probably isn't worth introducing more complexity.

// current
commands.spawn(SpriteBundle {
    transform: Transform::from_xyz(1.0, 0.0, 0.0),
    ..Default::default()  
 })
 
// nested bundle
commands.spawn(SpriteBundle {
    transform_bundle: TransformBundle {
        transform: Transform::from_xyz(1.0, 0.0, 0.0),
        ..Default::default()
    },
    ..Default::default()  
 })

@cart
Copy link
Member

cart commented Apr 12, 2021

I'm closing this for now because I think nesting for these core types is a net negative for UX. I do think we probably need to provide things like "transform bundles" to make it easier for users to define their own bundles in a way that is easy (no need to remember all of the components required) and update friendly (ex: when we add/remove something from NodeBundle, user bundles are updated).

And I see no reason why Bevy internals should be "special" here. People will feel the ergonomics implications of this api just as much as I currently do. If its "not ergonomic enough" for core plugins, its probably not ergonomic enough for user plugins!

I think this clearly needs more discussion and design work. Seems like a good opportunity for some RFCs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants