-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Migrate UI bundles to required components #15898
Migrate UI bundles to required components #15898
Conversation
This means we can't have The other open question here is the On the other hand though, |
Neither I find the |
Another option. |
Moving to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much to contribute here, mostly agreed with other comments.
I don't think UI components need Ui
in the name, since UiText
was heavily voted against, so there wouldn't be consistency either way
This is conceptually wrong. "Node" is the driving context. "Style" is a property of a "Node". The cost of training people to think otherwise is too high. The current port is already an ergonomic win. We can claim the remaining wins when we break up Style.
Me too. I prefer the short names. Instead of prefixing everything we should just pick concept names that are "unique enough", with a bias toward short and snappy to please the "ui ergonomics" crowd. I personally think
Yeah this seems reasonable. But we should discuss this elsewhere.
I still strongly believe Node should contain the "style properties that all nodes have" such as width, height, etc. But thats a post 0.15 conversation. |
…MaterialNode. Port to Required Components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a change that:
- Added support for BackgroundColor to UI material nodes, allowing
require(BackgroundColor)
to work as expected. (resolving the conversation above). - Renames
UiMaterialHandle
toMaterialNode
- Ports UI materials to Required Components.
MaterialNode
now requiresNode
(because it "is a" Node). - Deprecate
MaterialNodeBundle
I think this PR is now ready to merge
@@ -823,7 +842,7 @@ pub fn queue_uinodes( | |||
pipeline, | |||
entity: (*entity, extracted_uinode.main_entity), | |||
sort_key: ( | |||
FloatOrd(extracted_uinode.stack_index as f32), | |||
FloatOrd(extracted_uinode.stack_index as f32 + stack_z_offsets::BACKGROUND_COLOR), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just set extract_ui_material_nodes
after extract_ui_background_colors
instead.
TransparentUi uses a stable sort, so if two phase items have the same stack index then the z-order is determined by the ordering of the extraction systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. I don't have strong opinions one way or the other. I doubt a single per-entity float add will meaningfully tip the performance scales. Doing this per-entity gives us more granularity than "per thing encapsulated by a system", but we also currently have no use case where this matters. I also like that it puts the sort behavior front-and-center in a centralized place. The current behavior is pretty implicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't care about the performance impact but I don't like the inconsistancy very much and it breaks any existing code that rendered borders or text above ui materials on the same node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, but it also buys flexibility for future scenarios and decouples sort order from system execution order (which feels like a good thing as execution order could easily need to be different than sort order) .
I still don't have super strong opinions here. I think I prefer the explicitness of defining offsets. But I wouldn't strongly object to a change that moves this to using system execution order.
# Objective The `ui_material` recently gained a harsh red background via #15898. I'm assuming this was added for testing and forgotten about. https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/5268/compare/5259?screenshot=UI%20(User%20Interface)/ui_material.png ## Solution Remove the red background ## Testing `cargo run --example ui_material`
Objective
Solution
NodeBundle
in favor ofNode
ImageBundle
in favor ofUiImage
ButtonBundle
in favor ofButton
Testing
CI.
Migration Guide
NodeBundle
withNode
. e.g.ButtonBundle
withButton
. e.g.ImageBundle
withUiImage
. e.g.