Sprite3d #2814

VVishion · 2021-09-12T20:39:50Z

Objective

Make Sprites usable in 3d space.
Closes #2786.

Solution

Split TextureAtlasSprite into Sprite and TextureAtlasEntry. Sprites are bundled with either Texture (Handle<Image>) or TextureAtlas (Handle<TextureAtlas>) + TextureAtlasEntry.
Rename Sprite to Sprite2d and add Sprite3d.
Rename SpritePlugin to Sprite2dPlugin and add Sprite3dPlugin.
Make adjustments to the rendering pipeline.
Added Example 3d_sprite_pipelined

NOTE: I have not copied the color property from the removed TextureAtlasSprite since it wasn't used and wasn't on Sprite either. I think it's supposed to be on Sprite2d and Sprite3d, but that can obviously added later if needed.

# Objective Match cases of Sprite + Texture and Sprite + Texture from Atlas. ## Solution Textures for Sprites can either be defined by Texture or TextureAtlas + TextureAtlasEntry. Functionality from former TextureAtlasSprite is split into Sprite and TextureAtlasEntry now.

# Objective Make Sprites usable in 3d space. ## Solution Rename Sprite to Sprite2d and add Sprite3d. Add Example 3d_sprite_pipelined. NOTE: Currently, differences between Sprite2d and Sprite3d and its rendering pipelines are minor. Implementations might diverge to greater extend with future iterations.

# Objective Generalize shared behaviour for Sprite2d and Sprite3d. ## Solution Sprite2d and Sprite3d implementations might diverge to greater extend with future iterations. Make functions and structs generic where appropiate to keep the code base small, temporarily.

inodentry

Overall I wanna say I quite like this. :) The new API is nice and more consistent.

I also like the use of generics, to avoid code duplication, while still being able to "specialize" some functionality for 2d/3d.

I left some feedback for some changes, a few cosmetic things and one thing that seems like a bug/oversight.

inodentry · 2021-09-22T22:15:58Z

pipelined/bevy_sprite2/src/bundle.rs

@@ -29,20 +29,67 @@ impl Default for PipelinedSpriteBundle {
 /// A Bundle of components for drawing a single sprite from a sprite sheet (also referred
 /// to as a `TextureAtlas`)
 #[derive(Bundle, Clone)]
-pub struct PipelinedSpriteSheetBundle {
+pub struct Sprite2dSheetBundle {


This name reads very awkwardly. Maybe rename the type to SpriteSheet2dBundle instead?

Yap, thats better. Same for 3d as you noted below.

inodentry · 2021-09-22T22:16:19Z

pipelined/bevy_sprite2/src/bundle.rs

+/// A Bundle of components for drawing a single sprite from a sprite sheet (also referred
+/// to as a `TextureAtlas`)
+#[derive(Bundle, Clone)]
+pub struct Sprite3dSheetBundle {


inodentry · 2021-09-22T22:25:02Z

pipelined/bevy_sprite2/src/texture_atlas.rs

    pub index: u32,
-    pub flip_x: bool,
-    pub flip_y: bool,


I really like how this simplification resulted from including Sprite2d/Sprite3d in the sprite sheet bundles. Nice bit of code reuse.

inodentry · 2021-09-22T22:30:11Z

pipelined/bevy_sprite2/src/texture_atlas.rs

@@ -22,30 +22,19 @@ pub struct TextureAtlas {

 #[derive(Debug, Clone, TypeUuid, Reflect)]
 #[uuid = "7233c597-ccfa-411f-bd59-9af349432ada"]
-pub struct TextureAtlasSprite {
-    pub color: Color,


You said in the PR description that the implementation for sprite color was incomplete, and that's why you removed it. Do you mean that, previously, this field didn't do anything at all?

Just wanna make sure nothing is being regressed.

Yap, it wasn't used anywhere (most notably: not extracted to the render world). Same applies for flipping atm.

inodentry · 2021-09-22T22:35:50Z

pipelined/bevy_sprite2/src/sprite.rs

+    pub flip_y: bool,
+    /// An optional custom size for the sprite that will be used when rendering, instead of the size
+    /// of the sprite's image
+    pub custom_size: Option<Vec2>,


Now that Sprite2d/Sprite3d is a component inside the sprite sheet (texture-atlas-based) bundles, this custom_size field should be expected to work for those too.

It seems like it is currently broken, because I did not see this being handled in extract_atlases_{2d,3d}? You should fix those functions to account for this field, similar to extract_sprites_{2d,3d}.

P.S: also, I don't understand how sprite flipping is implemented, so I couldn't check that, but it's worth making sure that flip_x/flip_y are also handled correctly, for both standalone sprites as well as atlas/spritesheet sprites. Don't forget to test that. :)

I'll look into it. Maybe I've overlooked this.

IIRC, sprite flipping is not available for the new rendering pipeline atm. But I will also take a look at that.

Edit: flipping is not a property of the ExtractedSprite type. So it is not implemented currently. It was implemented in the old renderer and will probably be added back soon.

Flipping can be achieved in two ways if im correctly. Either swap the uv coords or flip the quad by 180 degrees only in the render world (since sprites are not culled) in the given direction. I don't know whats the proper implementation, though.

inodentry · 2021-09-22T22:44:39Z

pipelined/bevy_sprite2/src/render/mod.rs

+    }
+}
+
+impl FromWorld for SpriteShaders<Sprite3d> {


Just leaving a note to say that I haven't looked at this as part of my review of your PR, because I don't understand these rendering APIs well enough to review it.

The SpriteShaders are basically the same as before. The difference is that in the case of 3d we need to at least declare a depth attachment for the pipeline which the 2d one doesn't have (because no depth in 2d). This is due to how the render graph is modeled.

There is room for generalization/code recycling here. But I dont think this is worth the effort atm. This is only a basic implementation making sprite available in 3d. I haven't added any feature which are possible in 3d now (e.g. normals for 3d lighting which is not possible in 2d) so the pipelines might diverge more with future additions. Just to reason about my decision for the matter of code quality here.

inodentry · 2021-09-22T22:48:44Z

pipelined/bevy_sprite2/src/render/mod.rs

        extracted_sprites_res.sprites.extend(extracted_sprites);
    }
 }

-pub fn extract_sprites(
+pub fn extract_sprites_3d(


I wonder if this code really needs to be duplicated, for *_2d and *_3d? The bodies of the functions seem identical, except for the Sprite2d/Sprite3d types? Could/should this be made generic?

Say this would be generic. Now we have extract_sprites<T>. We query for &T. How do we access custom_size, flip_xor flip_y of any type T? So we have to implement a trait Sprite and use it as bounds for T. I just didn't find it worth the effort to generalize this if it probably diverges in the future as elaborated above (we might need to extract/prepare data for 3d which we do not for 2d).

VVishion · 2021-09-25T14:50:11Z

I'm going to rewrite once #2642 (scheduled for 0.6) is merged. I'll take into account things pointed out with the current iteration.

alice-i-cecile · 2021-11-05T23:57:16Z

@VVishion #3060 was just merged for you :)

inodentry · 2021-12-11T23:07:11Z

Actually, I am retracting my approval. I no longer think this is a good idea. This doesn't seem like a good direction for Bevy.

After some discussions on Discord with @cart and other community members, we came to the consensus that the development direction for bevy sprites should be towards diverging their api and implementation further from 3D, not unifying them / bringing them closer.

2D and 3D should have separate distinct APIs, so that users working on 2D games can have a better experience that feels "native to 2D". We don't want the engine to feel like it was designed for 3D first, or that 2D is a "second-class citizen" hacked together as a special case of the 3D stuff. For example, 2D users shouldn't have to think about 3D rotations and transforms. Bevy sprites might even move away from using the current Transform/GlobalTransform types.

Be more like Godot, less like Unity. :)

Additionally, having completely separate rendering for 2D may also allow for exploring more efficient and performant rendering techniques that are optimized for 2D. Bevy already moved away from drawing sprites simply as quads / 3D meshes. Sprites now have their own separate rendering pipeline configuration and shaders, and in the future, those will likely diverge even further from the 3D stuff.

I agree with you that it would probably be nice to have a nice-to-use "3D quads"/"billboards" renderer in Bevy. However, those are not the same as sprites, even though they might seem superficially similar. We shouldn't try to adapt the bevy sprites code to serve that use case. Instead, it should be a separate thing, so that each of these use cases can have nice APIs and implementations that are well-suited.

Maybe we should close this PR?

cart · 2021-12-11T23:46:33Z

I agree that the future of Bevy will likely have less overlap between 2d and 3d apis, but I do think its worth pointing out that Godot has a Sprite3d and still maintains a strict 2d/3d separation.

alice-i-cecile · 2021-12-12T05:21:40Z

Additionally, having completely separate rendering for 2D may also allow for exploring more efficient and performant rendering techniques that are optimized for 2D. Bevy already moved away from drawing sprites simply as quads / 3D meshes. Sprites now have their own separate rendering pipeline configuration and shaders, and in the future, those will likely diverge even further from the 3D stuff.

Strongly agree.

I agree with you that it would probably be nice to have a nice-to-use "3D quads"/"billboards" renderer in Bevy. However, those are not the same as sprites, even though they might seem superficially similar. We shouldn't try to adapt the bevy sprites code to serve that use case. Instead, it should be a separate thing, so that each of these use cases can have nice APIs and implementations that are well-suited.

I think I agree with this stance: their use cases and APIs and implementation should be related, but not identical. I think calling them Billboard is probably my preference: it clearly communicates their intent without tying them to the existing Sprite type.

As I understand it, there are actually 3 common patterns you tend to care about in 3D, which I'm giving arbitrary names for now:

Image, which is a 2D image drawn with a fixed orientation. This is what this PR implements.
Billboard, which is always drawn perpendicular to the camera. This is one of the UI-like behaviors, but not all billboards of this type are truly UI. These use Image as a primitive.
Decal, which conforms to a mesh. Probably basically unrelated to the above in terms of implementation, but something to be mindful of in terms of docs and design.

Maybe we should close this PR?

I think I'll second this: the existing work and example can be reused (and should be credited!), but I suspect it will be simpler to start fresh than worry about reverting all the renames.

alice-i-cecile · 2022-01-16T03:31:34Z

Closing this out. If you're interested in this work, please check out this PR and give some credit @VVishion :)

VVishion added 3 commits September 12, 2021 13:56

VVishion force-pushed the sprite3d branch from be52124 to af99c6c Compare September 12, 2021 20:45

alice-i-cecile added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible labels Sep 14, 2021

alice-i-cecile added the S-Needs-Review label Sep 22, 2021

inodentry suggested changes Sep 22, 2021

View reviewed changes

inodentry removed the S-Needs-Review label Sep 22, 2021

rodya-mirov mentioned this pull request Jan 16, 2022

Billboard sprites (Sprites in a 3D camera) #3688

Open

alice-i-cecile closed this Jan 16, 2022

makoConstruct mentioned this pull request May 7, 2022

Sprites in 3d #2786

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sprite3d #2814

Sprite3d #2814

VVishion commented Sep 12, 2021 •

edited

Loading

inodentry left a comment

inodentry Sep 22, 2021

VVishion Sep 25, 2021 •

edited

Loading

inodentry Sep 22, 2021

inodentry Sep 22, 2021

inodentry Sep 22, 2021

VVishion Sep 25, 2021 •

edited

Loading

inodentry Sep 22, 2021

VVishion Sep 25, 2021

VVishion Sep 25, 2021 •

edited

Loading

VVishion Sep 25, 2021 •

edited

Loading

inodentry Sep 22, 2021

VVishion Sep 25, 2021 •

edited

Loading

inodentry Sep 22, 2021

VVishion Sep 25, 2021 •

edited

Loading

VVishion commented Sep 25, 2021 •

edited

Loading

alice-i-cecile commented Nov 5, 2021

inodentry commented Dec 11, 2021

cart commented Dec 11, 2021 •

edited

Loading

alice-i-cecile commented Dec 12, 2021

alice-i-cecile commented Jan 16, 2022

Sprite3d #2814

Sprite3d #2814

Conversation

VVishion commented Sep 12, 2021 • edited Loading

Objective

Solution

inodentry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VVishion Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VVishion Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VVishion Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

VVishion Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VVishion Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VVishion Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

VVishion commented Sep 25, 2021 • edited Loading

alice-i-cecile commented Nov 5, 2021

inodentry commented Dec 11, 2021

cart commented Dec 11, 2021 • edited Loading

alice-i-cecile commented Dec 12, 2021

alice-i-cecile commented Jan 16, 2022

VVishion commented Sep 12, 2021 •

edited

Loading

VVishion Sep 25, 2021 •

edited

Loading

VVishion Sep 25, 2021 •

edited

Loading

VVishion Sep 25, 2021 •

edited

Loading

VVishion Sep 25, 2021 •

edited

Loading

VVishion Sep 25, 2021 •

edited

Loading

VVishion Sep 25, 2021 •

edited

Loading

VVishion commented Sep 25, 2021 •

edited

Loading

cart commented Dec 11, 2021 •

edited

Loading