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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Slight perf improvements and tidy for contributors example #3764

Closed
wants to merge 13 commits into from
Closed

[Merged by Bors] - Slight perf improvements and tidy for contributors example #3764

wants to merge 13 commits into from

Conversation

SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented Jan 25, 2022 •
edited by bors bot
Loading

No description provided.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 25, 2022
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples and removed S-Needs-Triage This issue needs to be labelled labels Jan 25, 2022
@alice-i-cecile
Copy link
Member

I generally really like these changes! They clean things up a bit, and are well-motivated. In real code, I would probably prefer the for_each pattern for perf, but the for loop is easier to read for beginners (and Cart has asked us to use that style in examples).

@SUPERCILEX
Copy link
Contributor Author

@alice-i-cecile Is there anything I need to do after the approval?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 27, 2022 •
edited
Loading

@alice-i-cecile Is there anything I need to do after the approval?

Nope, you're all good. If you'd like, you can ask around and wrangle up another approval from the community. Once that's in place, I can merge this in.

(I have merge rights for docs and examples).

@SUPERCILEX
Copy link
Contributor Author

Makes sense, thanks!

@rparrett
Copy link
Contributor

While we're at it, it would be a bit more idiomatic for that timer to be a resource rather than a component.

@SUPERCILEX
Copy link
Contributor Author

@rparrett Like this? 52d57fa

@rparrett
Copy link
Contributor

@rparrett Like this? 52d57fa

That looks a lot nicer!

alice-i-cecile
alice-i-cecile approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I like the further changes! A bit iffy on the clippy ignore, but I agree that it's a clippy bug.

@SUPERCILEX SUPERCILEX mentioned this pull request Feb 24, 2022
Closed
@SUPERCILEX
Go back to simpler assignment
bfc822c 
Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX
Merge remote-tracking branch 'upstream/main' into tmp2
6e82da4 
# Conflicts:
#	examples/2d/contributors.rs
@SUPERCILEX
Fix merge mistakes
399fe3b 
Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX
Copy link
Contributor Author

@alice-i-cecile @rparrett Looks like this should finally be ready for a merge. Any outstanding concerns?

@rparrett
Copy link
Contributor

rparrett commented Apr 8, 2022

Just a nit, but a few other examples (plugin, font_atlas_debug) use a naming scheme that would look like

struct SelectionState {
    timer: Timer,
    has_triggered: bool,
}

When combining a timer with some other state in a resource.

@SUPERCILEX
Naming changes
219ce3b 
Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX
Copy link
Contributor Author

Sounds good, done!

rparrett
rparrett approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@rparrett rparrett left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This changeset seems like an improvement to me. I tested with an artificially shortened contributor list to ensure that the wrapping still works.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Apr 8, 2022
@SUPERCILEX
Slight perf improvements and tidy for contributors example (#3764)
f23ae10
@bors
Copy link
Contributor

bors bot commented Apr 8, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Slight perf improvements and tidy for contributors example [Merged by Bors] - Slight perf improvements and tidy for contributors example Apr 8, 2022
@bors bors bot closed this Apr 8, 2022
@SUPERCILEX
Copy link
Contributor Author

Woohoo, thanks!

@SUPERCILEX SUPERCILEX deleted the tmp2 branch May 17, 2022 04:21
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@SUPERCILEX @aevyrie
Slight perf improvements and tidy for contributors example (bevyengin…
2e1c9cf 
…e#3764)
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@SUPERCILEX @ItsDoot
Slight perf improvements and tidy for contributors example (bevyengin…
d05817f 
…e#3764)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@rparrett rparrett rparrett approved these changes

Assignees
No one assigned
Labels
C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SUPERCILEX @alice-i-cecile @rparrett