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

Resolve UI outlines using the correct target's viewport size #14947

Merged
merged 7 commits into from
Sep 2, 2024

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Aug 27, 2024

Objective

resolve_outlines_system wasn't updated when multi-window support was added and it always uses the size of the primary window when resolving viewport coords, regardless of the layout's camera target.

Fixes #14945

Solution

It's awkward to get the viewport size of the target for an individual node without walking the tree or adding extra fields to Node, so I removed resolve_outlines_system and instead the outline values are updated in ui_layout_system.

…n `ui_layout_system` instead.

* Resolve outlines using the target's viewport size, instead of always using the size of the `PrimaryWindow`.
@ickshonpe ickshonpe changed the title Outlines Fix Resolve UI outlines using the correct target's viewport size Aug 27, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 27, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Aug 27, 2024
@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.14.2 Aug 27, 2024
@ickshonpe
Copy link
Contributor Author

Not sure why the CI is failing with all those system ordering amibiguities.

@tychedelia
Copy link
Contributor

Not sure why the CI is failing with all those system ordering amibiguities.

The CI is difficult to parse, you need to look for systems you specifically touched.

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Seems simpler.

@alice-i-cecile
Copy link
Member

Yeah, I'm not thrilled with the output, but if the net effect is a decrease in ambiguities just set the expected number to the new total.

@alice-i-cecile alice-i-cecile 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 Aug 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 2, 2024
Merged via the queue into bevyengine:main with commit be100b8 Sep 2, 2024
26 checks passed
mockersf pushed a commit that referenced this pull request Sep 5, 2024
`resolve_outlines_system` wasn't updated when multi-window support was
added and it always uses the size of the primary window when resolving
viewport coords, regardless of the layout's camera target.

Fixes #14945

It's awkward to get the viewport size of the target for an individual
node without walking the tree or adding extra fields to `Node`, so I
removed `resolve_outlines_system` and instead the outline values are
updated in `ui_layout_system`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI outline geometry is always resolved using the size of the PrimaryWindow regardless of target
3 participants