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

UI Texture Atlas support #3792

Closed

Conversation

ManevilleF
Copy link
Contributor

@ManevilleF ManevilleF commented Jan 28, 2022

Objective

Adds support for texture atlas in bevy_ui Nodes

Solution

  • I created a custom UiTextureAtlas component with a simple texture index and a Handle<bevy_sprite::TextureAtlas>
  • I plugged a new extraction system in the render pipeline
  • I added new UI Bundles using UiTextureAtlas
  • I added an example
  • I added an extra system for the ImageMode support

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 28, 2022
@ManevilleF ManevilleF changed the title UI Texture Atlas support Draft: UI Texture Atlas support Jan 28, 2022
@ManevilleF ManevilleF changed the title Draft: UI Texture Atlas support UI Texture Atlas support Jan 28, 2022
@ManevilleF ManevilleF force-pushed the feat/bevy_ui_improvements branch from ceb555a to 1cc7bd2 Compare January 29, 2022 09:19
@NiklasEi NiklasEi added A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Jan 29, 2022
@alice-i-cecile alice-i-cecile added the C-Feature A new feature, making something new possible label May 16, 2022
@@ -54,6 +55,29 @@ pub struct ImageBundle {
pub visibility: Visibility,
}

/// A UI node that is an image with texture sheet
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// A UI node that is an image with texture sheet
/// A UI node that displays an image stored in a texture sheet

pub image_mode: ImageMode,
/// The calculated size based on the given image
pub calculated_size: CalculatedSize,
/// The color of the node
Copy link
Member

Choose a reason for hiding this comment

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

This should describe how the blending is done.

@@ -132,6 +156,48 @@ impl Default for ButtonBundle {
}
}

/// A UI node that is a button with a texture sheet
#[derive(Bundle, Clone, Debug)]
pub struct ButtonSheetBundle {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the creation of a dedicated bundle here: this should be done compositionally.

pub visibility: Visibility,
}

impl Default for ButtonSheetBundle {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the derive work fine here?

@@ -82,12 +82,22 @@ pub fn build_ui_render(app: &mut App) {
.add_system_to_stage(RenderStage::Extract, extract_ui_camera_phases)
.add_system_to_stage(
RenderStage::Extract,
// This system is the first of `RenderStage::Extract` it is responsible for both:
Copy link
Member

@alice-i-cecile alice-i-cecile May 16, 2022

Choose a reason for hiding this comment

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

This should be on doc strings for extract_uinodes and RenderUiSystem instead.

@@ -172,6 +184,61 @@ pub fn extract_uinodes(
}
}

pub fn extract_atlas_uinodes(
Copy link
Member

Choose a reason for hiding this comment

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

This is public, and thus needs doc strings.

/// The texture atlas of the node
#[derive(Component, Clone, Debug, Default, Reflect)]
#[reflect(Component)]
pub struct UiTextureAtlas {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel UI specific at all. Why can't we just use TextureAtlasSprite + Handle<TextureAtlas>?

Whenever possible, we should try to avoid duplicated structures between UI and 2D to avoid divergence and confusion.

@@ -0,0 +1,80 @@
use bevy::prelude::*;

/// This example illustrates how to create a button that changes color and text based on its
Copy link
Member

Choose a reason for hiding this comment

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

This should be a module-style comment: see #4438.

@@ -243,6 +243,7 @@ Example | File | Description
Example | File | Description
--- | --- | ---
`button` | [`ui/button.rs`](./ui/button.rs) | Illustrates creating and updating a button
`button_with_atlas` | [`ui/button_with_atlas.rs`](./ui/button_with_atlas.rs) | Illustrates creating and updating a button with a texture atlas
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we could roll this into the button.rs example, and just create two different buttons there.

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.

Some nits, and one major objection: I think that whenever possible we should try to reuse and unify code between UI and 2D.

I agree with the direction here and think it's quite useful. Can you add more context to the Objective section of your PR explaining what end users might use this feature for in practice?

My instinct was that you could use this for animated buttons (including changes on e.g. hover), and @inodentry pointed out that you could use this to enable reuse of texture atlases across the game content and UI, like in menus.

@ManevilleF
Copy link
Contributor Author

Thanks for the review, I completely forgot about this PR.

I think I'll redo it completely from the main branch following your remarks.

About the use cases I see both:

  • Animated buttons (and anything really)
  • Global texture mapping for UI

@ManevilleF
Copy link
Contributor Author

@alice-i-cecile I'm closing this in favor of #5070

@ManevilleF ManevilleF closed this Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants