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] - Add sprite atlases into the new renderer. #2560

Closed
wants to merge 8 commits into from

Conversation

StarArawn
Copy link
Contributor

@StarArawn StarArawn commented Jul 28, 2021

Objective

Restore the functionality of sprite atlases in the new renderer.

Note: This PR relies on #2555

Solution

Mostly just a copy paste of the existing sprite atlas implementation, however I unified the rendering between sprites and atlases.

size: sprite.size,
rect: Rect {
min: Vec2::ZERO,
max: sprite.size,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's worth pointing out that this actually changes existing behavior for sprite.size in the fact that it now indicates a pixel texel size instead of a sprite dimension size. Instead it might be better to make Rect optional here?

@cart cart added the A-Rendering Drawing game state to the screen label Jul 28, 2021
@cart cart added this to the Bevy 0.6 milestone Jul 28, 2021
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Love how simple this impl ended up being / how well it slots into the existing impl.

}
}

if let Some(mut extracted_sprites_res) = render_world.get_resource_mut::<ExtractedSprites>() {
Copy link
Member

Choose a reason for hiding this comment

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

If you do app.init_resource::<ExtractedSprites>() in the sprite plugin, you can skip this check and just assume it exists (same goes for the other extract system).

if let Some(mut extracted_sprites_res) = render_world.get_resource_mut::<ExtractedSprites>() {
extracted_sprites_res.sprites.extend(extracted_sprites);
} else {
render_world.insert_resource(ExtractedSprites {
Copy link
Member

@cart cart Jul 30, 2021

Choose a reason for hiding this comment

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

Unless we clear ExtractedSprites somewhere, this will collect sprites every frame. The commands.insert_resource approach used elsewhere overwrites the old value, which does the clearing "automatically".

Copy link
Member

Choose a reason for hiding this comment

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

The downside being that we don't reuse allocations across frames.


#[derive(Debug, Clone, TypeUuid, Reflect)]
#[uuid = "7233c597-ccfa-411f-bd59-9af349432ada"]
#[repr(C)]
Copy link
Member

Choose a reason for hiding this comment

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

this no longer needs repr(C) because it doesn't need to be byteable

@StarArawn StarArawn requested a review from cart July 31, 2021 21:08
@cart
Copy link
Member

cart commented Aug 4, 2021

Looks good to me!

@cart
Copy link
Member

cart commented Aug 4, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 4, 2021
# Objective
Restore the functionality of sprite atlases in the new renderer.

### **Note:** This PR relies on #2555 

## Solution
Mostly just a copy paste of the existing sprite atlas implementation, however I unified the rendering between sprites and atlases.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 4, 2021

@bors bors bot changed the title Add sprite atlases into the new renderer. [Merged by Bors] - Add sprite atlases into the new renderer. Aug 4, 2021
@bors bors bot closed this Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants