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

Add UiImage offset property #5629

Closed
wants to merge 6 commits into from

Conversation

afonsolage
Copy link
Contributor

@afonsolage afonsolage commented Aug 9, 2022

Objective

Currently UiImage component is just a unit struct for Handle<Image> . At it's core, it draws the image and that's it. If someone needs to have a more complex use case, like implementing a nine patch, one have to use TextureAtlas which is good, but is a overkill for ui works.

This PR is related to #5070, but it aims to make TextureAtlas to work with UiImage which is a different use case IMO, since one should use TextureAtlas and UiImage together on same entity.

Solution

Add an offset property to UiImage and enable to render a portion of the image, much like a TexturaAtlas but simplified, so users may create it's own image atlas and reuse on many UiImage components.

Changelog

  • Added offset property on UiImage and changed to from unit struct to normal struct;
  • Removed Deref and DerefMut from UiImage since it's no longer a unit struct;
  • Changed extract_uinodes to use UiImage::offset when it's non-zero;
  • Added image_button example:
    image_button

Migration Guide

UiImage is now a normal struct so it can't be directly dereferenced anymore and should use normal struct initialization.

@IceSentry IceSentry added A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible labels Aug 9, 2022
@alice-i-cecile
Copy link
Member

Ping @ManevilleF for review here :)

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 9, 2022

fn main() {
App::new()
// Change image filter to a pixel-art friendly
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
// Change image filter to a pixel-art friendly
// Change image filter to a pixel-art friendly mode

}

// Image rect in pixels, inside the base image.
// Values are using to built a rect with begin (X, Y) and end (X, Y) format
Copy link
Member

Choose a reason for hiding this comment

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

Users would really benefit from some more advice on how these values were determined.

align_items: AlignItems::Center,
..default()
},
// Default image has no offset, but that's OK since it'll be update it on button_system
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
// Default image has no offset, but that's OK since it'll be update it on button_system
// Default image has no offset, but that's OK since it'll be updated in button_system

@alice-i-cecile
Copy link
Member

Code quality and docs seem fine, minus a few nits. I'm not fully sold on the grand vision here, so I'll defer to @mockersf and @ManevilleF who've thought about this space more.

@ManevilleF
Copy link
Contributor

ManevilleF commented Aug 9, 2022

The new property offset should be called rect and I think it should be a separate component, overriding the default behaviour to be more ECS friendly.

But to customize Rect of a texture is what a TextureAtlas is for, and I think #5103 + #5072 is the way, as UI and sprites shouldn't behave differently in that scope

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Aug 9, 2022
@mockersf
Copy link
Member

mockersf commented Aug 9, 2022

I can't do a complete review anytime soon, but anything that impact rendered size should impact layout, and I didn't see layout files being modified in this PR

@alice-i-cecile
Copy link
Member

Closing in favor of #5103; let's collect our efforts there.

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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants