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

[bevy_ui/layout] Add tests, missing asserts, and missing debug fields for UiSurface #12803

Conversation

StrikeForceZero
Copy link
Contributor

@StrikeForceZero StrikeForceZero commented Mar 30, 2024

This is 3 of 5 iterative PR's that affect bevy_ui/layout


Objective

  • Add tests to UiSurface
  • Add missing asserts in _assert_send_sync_ui_surface_impl_safe
  • Add missing Debug field print for camera_entity_to_taffy

Solution

  • Adds tests to UiSurface
  • Adds missing asserts in _assert_send_sync_ui_surface_impl_safe
  • Adds missing impl Debug field print for camera_entity_to_taffy

@NthTensor NthTensor added A-UI Graphical user interfaces, styles, layouts, and widgets C-Testing A change that impacts how we test Bevy or how users test their apps labels Mar 30, 2024
@StrikeForceZero StrikeForceZero force-pushed the dev/bevy_ui/ui_surface_tests_and_missing branch from 2cbf62b to d65c0b2 Compare March 30, 2024 18:23
@james7132 james7132 requested a review from ickshonpe April 3, 2024 02:48
@StrikeForceZero StrikeForceZero force-pushed the dev/bevy_ui/ui_surface_tests_and_missing branch 2 times, most recently from cc38996 to 4159fe0 Compare April 3, 2024 05:49
@StrikeForceZero StrikeForceZero force-pushed the dev/bevy_ui/ui_surface_tests_and_missing branch from 4159fe0 to 14bcd1e Compare April 30, 2024 16:19
@StrikeForceZero StrikeForceZero force-pushed the dev/bevy_ui/ui_surface_tests_and_missing branch from 14bcd1e to 7e39ce8 Compare April 30, 2024 16:21
@StrikeForceZero StrikeForceZero force-pushed the dev/bevy_ui/ui_surface_tests_and_missing branch from 3df38c1 to 0f0e8bd Compare April 30, 2024 19:25
@StrikeForceZero StrikeForceZero force-pushed the dev/bevy_ui/ui_surface_tests_and_missing branch from 95a690a to 220601e Compare April 30, 2024 22:48
StrikeForceZero added a commit to StrikeForceZero/bevy that referenced this pull request May 19, 2024
Apply rustfmt
Move test_initialization next to other tests
Add doc comment to is_root_node_pair_valid
Move helper methods into the single test case where they are used
Add tests for helper methods
Remove trait helpers to reduce complexity of tests
Mark specific test functions as unreachable
Move ui_surface test only methods into mod tests as trait
Fix tests after rebase
Add tests for bevy_ui/layout/ui_surface
Widen type for parameter children in UiSurface::update_children
Add missing asserts and Debug fields in UiSurface from bevyengine#12268 and bevyengine#12698
@StrikeForceZero StrikeForceZero changed the title [bevy_ui/layout] Adds tests and missing asserts to UiSurface [bevy_ui/layout] Add tests, missing asserts, missing debug field, and widen children param in update_children for UiSurface May 19, 2024
@msvbg msvbg added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 13, 2024
@StrikeForceZero
Copy link
Contributor Author

For now, I've excluded the one test, test_compute_camera_layout, when bevy_text is active inside layout/ui_surface::tests.
For coverage of those new bevy_text params, I went with the path of least resistance by adding the test inside layout/mod::tests since it already has a Bevy app being created.

But I'm not sure I like layout/mod owning responsibility for layout/ui_surface. Since that was the whole point of extracting ui_surface to its own file; Any suggestions? The one thing that comes to mind would be moving setup_ui_test_world to a pub mod tests {} and exposing it as pub for ui_surface to call


I removed the change described as "Widen children param in update_children (required for tests)" because the new GhostNode changes rely on it continuing to be an iterator, and my previous reasoning is likely flawed.


Previously I was using

const TEST_LAYOUT_CONTEXT: LayoutContext = LayoutContext {
    scale_factor: 1.0, // default 1.0
    physical_size: bevy_math::Vec2::ONE, // default Vec2(0,0)
    min_size: 0.0, // default 0.0
    max_size: 1.0, // default 0.0
};

but changed to using LayoutContext::default(). Does anyone know if the default values nullify the usefulness of the tests? They still seem to pass, but it's been several months since I initially worked on this, so I'm not acutely aware of any side effects.


Otherwise, the only outstanding item is this discussion: #12803 (comment)

@UkoeHB
Copy link
Contributor

UkoeHB commented Oct 3, 2024

Does anyone know if the default values nullify the usefulness of the tests?

I have not looked at the tests in detail, but the default LayoutContext has zero size, so presumably there is a lot of short-circuiting inside the layout algorithm which probably invalidates the test results. I'd revert back to using TEST_LAYOUT_CONTEXT but use a larger max_size, more like 1000.0.

@StrikeForceZero
Copy link
Contributor Author

I'd revert back to using TEST_LAYOUT_CONTEXT but use a larger max_size, more like 1000.0.

Good idea, restored in 03c2f34

@pablo-lua pablo-lua 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 Oct 12, 2024
@alice-i-cecile
Copy link
Member

Thanks for your patience <3 Really nice stuff.

@alice-i-cecile
Copy link
Member

@StrikeForceZero this needs to be manually updated to reflect the changes on main. Sorry :( I really like this work and would love to get it in though, so please let us know if you're interested in completing this.

@StrikeForceZero StrikeForceZero changed the title [bevy_ui/layout] Add tests, missing asserts, missing debug field, and widen children param in update_children for UiSurface [bevy_ui/layout] Add tests, missing asserts, and missing debug fields in for UiSurface Oct 15, 2024
@StrikeForceZero StrikeForceZero changed the title [bevy_ui/layout] Add tests, missing asserts, and missing debug fields in for UiSurface [bevy_ui/layout] Add tests, missing asserts, and missing debug fields for UiSurface Oct 15, 2024
@StrikeForceZero
Copy link
Contributor Author

@StrikeForceZero this needs to be manually updated to reflect the changes on main.

Ok, should be good now @alice-i-cecile

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 15, 2024
Merged via the queue into bevyengine:main with commit de08fb2 Oct 15, 2024
28 checks passed
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-Testing A change that impacts how we test Bevy or how users test their apps 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.

7 participants