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] - Added colors to sprite stress test #5317

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Added colors to sprite stress test #5317

wants to merge 4 commits into from

Conversation

ManevilleF
Copy link
Contributor

Objective

Allow better performance testing for #5247

Solution

I added color tints to the many_sprites example stress test.

@ManevilleF
Copy link
Contributor Author

@superdump This should be enough to test #5247

@mockersf
Copy link
Member

How noticeable is the perf difference with this PR?

Would it make sense to add colors only under a flag if we want to continue testing rendering without colors?

@ManevilleF
Copy link
Contributor Author

Oh the performance is terrible, on my computer it jumps from 20 to 10 FPS

@superdump
Copy link
Contributor

I think this should be implemented as a command line option as we will want to test different things.

Do you mean that this PR gets 20fps and #5247 with this PR gets 10? Or main vs this PR?

@ManevilleF
Copy link
Contributor Author

I added the command line option.

@superdump what I meant is that on this PR, so basically main, the color tinting reduces the framerate by 50%. Completely unrelated to #5247 . Once this PR is merged I will rebase and test for #5247

superdump
superdump requested changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Minor additional clarification to the purpose of —colored.

examples/stress_tests/many_sprites.rs Outdated Show resolved Hide resolved
@superdump
Copy link
Contributor

@superdump what I meant is that on this PR, so basically main, the color tinting reduces the framerate by 50%. Completely unrelated to #5247 .

Ok, that’s expected.

@ManevilleF
Copy link
Contributor Author

Now that I think about it we have no way to know how many batches are created since the color tinting is random. So I don't know if this stress test can be useful to test rendering performance as we can stumble upon a very favorable layout or a very unfavorable one which could mess up the test results

@superdump
Copy link
Contributor

In all likelihood there will be many batches. You can see how many draw commands are created. Or after the sort/batch stage, at the beginning of the render stage, see how many phase items there are, I guess. As they get processed into draw commands

@ManevilleF
Copy link
Contributor Author

In all likelihood there will be many batches. You can see how many draw commands are created. Or after the sort/batch stage, at the beginning of the render stage, see how many phase items there are, I guess. As they get processed into draw commands

So should I add something or do you think this is this enough ?

@superdump
Copy link
Contributor

I think it’s enough. We should add rendering diagnostics elsewhere. But we should. :)

mockersf
mockersf approved these changes Jul 14, 2022
View reviewed changes
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

your color choice seems fitting for today

@mockersf mockersf added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples C-Performance A change motivated by improving speed, memory usage or compile times labels Jul 14, 2022
superdump
superdump approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot pushed a commit that referenced this pull request Jul 14, 2022
@ManevilleF
Added colors to sprite stress test (#5317)
dc3b4b6 
# Objective

Allow better performance testing for #5247


## Solution

I added color tints to the `many_sprites` example stress test.
@bors
Copy link
Contributor

bors bot commented Jul 14, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Added colors to sprite stress test [Merged by Bors] - Added colors to sprite stress test Jul 14, 2022
@bors bors bot closed this Jul 14, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
@ManevilleF @inodentry
Added colors to sprite stress test (bevyengine#5317)
5cb6089 
# Objective

Allow better performance testing for bevyengine#5247


## Solution

I added color tints to the `many_sprites` example stress test.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@ManevilleF @james7132
Added colors to sprite stress test (bevyengine#5317)
5b6dde2 
# Objective

Allow better performance testing for bevyengine#5247


## Solution

I added color tints to the `many_sprites` example stress test.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@ManevilleF @ItsDoot
Added colors to sprite stress test (bevyengine#5317)
3fc2d07 
# Objective

Allow better performance testing for bevyengine#5247


## Solution

I added color tints to the `many_sprites` example stress test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@superdump superdump superdump approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ManevilleF @mockersf @superdump