Skip to content

InlineImage custom size and flipping support - #25767

Merged
alice-i-cecile merged 12 commits into
bevyengine:mainfrom
ickshonpe:inline-image-size
Sep 15, 2026
Merged

alice-i-cecile merged 12 commits into
bevyengine:mainfrom
ickshonpe:inline-image-size

Conversation

@ickshonpe

@ickshonpe ickshonpe commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Objective

Add support for custom sizing and flipping to InlineImage.

Solution

New fields width, height, flip_x, flip_y on IinlineImage.

Preserves the image's aspect ratio if dimensions unset.


Probably the dimensions should be Vals but the implementation needs a bit of thinking about, so left it for a follow up

Testing

Includes new test resolve_inline_box_size.

Updated the inline_image example and inlineimage ui testbed scene.

@ickshonpe
ickshonpe requested a review from viridia September 13, 2026 10:37
@ickshonpe ickshonpe added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 13, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in UI Sep 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-25767

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

@ickshonpe ickshonpe added the M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered label Sep 13, 2026

@viridia viridia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fine for now, but I assume we eventually want these to be Val units. (Particularly Em/Rem).

@ickshonpe

ickshonpe commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Fine for now, but I assume we eventually want these to be Val units. (Particularly Em/Rem).

Yep I left Vals for a follow up as I wasn't quite sure what to do some of the variants:

  • Em: which Em, the root's, parent's, or one of the adjacent sibling text sections'?
  • Percent would need to based on something, can't be the size of the text node or line height as it would be cyclic. If it's Em same problem as for Em above. Could be based on image size or Rem size but neither seems natural?

Could use FontSize instead of Val, as it has no Percent variant. But setting an image size using FontSize seems weird too.
It may just be a case where there's no happy solution, just have to pick the least worst option and highlight the weird parts in the docs.

@viridia

viridia commented Sep 13, 2026

Copy link
Copy Markdown
Contributor
  • Em: which Em, the root's, parent's, or one of the adjacent sibling text sections'?

It doesn't really matter; root is probably easier and more consistent.

The practical use case is: "I want an icon that is 20% larger than the text height of the paragraph it's embedded in". Having a paragraph contained mixed sizes is rare; generally font size applies to an entire heading or caption.

impl InlineImage {
/// Resolve the inline box size. Preserves the image's aspect ratio if dimensions unset.
pub fn resolve_inline_box_size(&self, image_size: Vec2) -> Vec2 {
match (self.width, self.height) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Probably dumb question but is zero a valid/possible image size here?

@ickshonpe ickshonpe Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No that's a reasonable question. I don't think this function should do any validation, that should be left up to the caller. Instead, I added some extra logic to the update_inline_image_boxes system that sets the InlineBox to zero if the resolved box size is invalid. IIRC it's not strictly necessary, images shouldn't be zero-sized and Parley will ignore any invalid values, but it seems okay as a low cost defensive measure.

commands.spawn(Camera2d);
commands.spawn((
Text::new("[Text]"),
TextFont::from_font_size(px(40.)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just for my learning what's the easiest way to see the new examples on an open PR? Not sure if screenshots in PR desc. are common practice to make it easier for reviewers.

@ickshonpe ickshonpe Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sometimes I add screenshots, but they can be a bit unreliable. It's much better if reviewers run the examples and check that they work for themselves. I've had a couple of PRs where the changes were fine on my computer, but on other platforms there were graphical artifacts.

The examples here can be run with:

cargo run --example inline_image
cargo run --example testbed_ui -- inlineimage

@ickshonpe

Copy link
Copy Markdown
Contributor Author
  • Em: which Em, the root's, parent's, or one of the adjacent sibling text sections'?

It doesn't really matter; root is probably easier and more consistent.

The practical use case is: "I want an icon that is 20% larger than the text height of the paragraph it's embedded in". Having a paragraph contained mixed sizes is rare; generally font size applies to an entire heading or caption.

Yeah I think I agree, we can just simplify things by assuming a uniform line height (taken from the root text section). I was thinking about how to implement automatic sizing that would adjust the image size to best fit the text layout, but it's not worth the effort for something very niche, however unsatisfying the alternative is.

I'll still leave it for a follow up, as this is very simple and an obvious merge.

@ickshonpe ickshonpe added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 15, 2026
@alice-i-cecile
alice-i-cecile added this pull request to the merge queue Sep 15, 2026
Merged via the queue into bevyengine:main with commit f681cef Sep 15, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in UI Sep 15, 2026
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants