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

[Merged by Bors] - Cleanup many sprites stress tests #7436

Closed
wants to merge 2 commits into from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jan 31, 2023

Since the new renderer, no frustum culling is applied to 2d components
(be it Sprite or Mesh2d), the stress_tests docs is therefore misleading
and should be updated.

Furthermore, the many_animated_sprites example, unlike many_sprites
kept vsync enabled, making the stress test less useful than it could be.
We now disable vsync for many_animated_sprites.

Also, many_animated_sprites didn't have the stress_tests warning
message, instead, it had a paragraph in the module doc. I replaced the
module doc paragraph by the warning message, to be more in line with
other examples.

Solution

  • Remove the paragraph about frustum culling in the many_sprites
    and many_animated_sprites stress tests

@nicopap nicopap added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples labels Jan 31, 2023
@rparrett
Copy link
Contributor

rparrett commented Jan 31, 2023

Are there no longer plans for adding 2d frustum culling? If I'm remembering right, we only briefly had a buggy implementation (not sure if it was merged actually), but many_sprites was added to test it.

There's a stalled out PR here: #3944. It seems potentially worth doing.

Could the text be tweaked slightly so that it doesn't imply that this is something we're already doing?

@nicopap
Copy link
Contributor Author

nicopap commented Jan 31, 2023

@rparrett Does make sense. Furthermore #3944 seems both a clear win and still fairly easy to implement. I'll reword it.

@nicopap nicopap marked this pull request as draft February 1, 2023 07:10
@nicopap nicopap changed the title Remove references to frustrum culling in 2d tests Cleanup many sprites stress tests Feb 1, 2023
@nicopap nicopap marked this pull request as ready for review February 1, 2023 08:48
Since the new renderer, no frustum culling is applied to 2d components
(be it Sprite or Mesh2d), the stress_tests docs is therefore misleading
and should be updated.

Furthermore, the `many_animated_sprites` example, unlike `many_sprites`
kept vsync enabled, making the stress test less useful than it could be.
We now disable vsync for `many_animated_sprites`.

Also, `many_animated_sprites` didn't have the stress_tests warning
message, instead, it had a paragraph in the module doc. I replaced the
module doc paragraph by the warning message, to be more in line with
other examples.
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 1, 2023
@alice-i-cecile
Copy link
Member

@nicopap CI appears to be failing due to a broken import. Once that's fixed, I'm happy to merge this.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 1, 2023
Since the new renderer, no frustum culling is applied to 2d components
(be it Sprite or Mesh2d), the stress_tests docs is therefore misleading
and should be updated.

Furthermore, the `many_animated_sprites` example, unlike `many_sprites`
kept vsync enabled, making the stress test less useful than it could be.
We now disable vsync for `many_animated_sprites`.

Also, `many_animated_sprites` didn't have the stress_tests warning
message, instead, it had a paragraph in the module doc. I replaced the
module doc paragraph by the warning message, to be more in line with
other examples.

## Solution

- Remove the paragraph about frustum culling in the `many_sprites`
  and `many_animated_sprites` stress tests
@bors bors bot changed the title Cleanup many sprites stress tests [Merged by Bors] - Cleanup many sprites stress tests Feb 1, 2023
@bors bors bot closed this Feb 1, 2023
@nicopap nicopap deleted the do-not-lie-about-culling branch August 30, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation C-Examples An addition or correction to our examples 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.

4 participants