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

Simplify text system queries #8547

Closed
wants to merge 8 commits into from

Conversation

ickshonpe
Copy link
Contributor

Objective

Both measure_text_system and text_system have ParamSet parameters each with three text queries. This complexity doesn't seem necessary and the ParamSet queries can be replaced with a single combined query.

Solution

Replace the ParamSets in measure_text_system and text_system with single combined queries.


Changelog

  • Replaced the measure_texture_system and text_system ParamSet parameters with single combined queries.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented May 5, 2023

Seems marginally more efficient as well:

cargo run --example many_buttons --profile stress-test --features trace_tracy -- recompute-text

text_system-many_buttons-stress-test-a616fa8f70054d1c504bc-vs-baece543878

@nicoburns nicoburns added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change labels May 5, 2023
@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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 5, 2023
Copy link
Contributor

@konsti219 konsti219 left a comment

Choose a reason for hiding this comment

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

Using iterators is a bit shorter than a for loop

Comment on lines +87 to 89
for (entity, ..) in text_query.iter() {
text_queue.push(entity);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (entity, ..) in text_query.iter() {
text_queue.push(entity);
}
text_queue.extend(text_query.iter().map(|(entity, ..)| entity));

Comment on lines +162 to 164
for (entity, ..) in text_query.iter() {
text_queue.push(entity);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (entity, ..) in text_query.iter() {
text_queue.push(entity);
}
text_queue.extend(text_query.iter().map(|(entity, ..)| entity));

@ickshonpe
Copy link
Contributor Author

I needed to include these changes in #8549, so I might as well close this if #8549 isn't controversial or blocked by anything.

@ickshonpe ickshonpe closed this May 5, 2023
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-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants