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] - fix cluster tiling calculations #4148

Closed
wants to merge 8 commits into from

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Mar 8, 2022

Objective

fix cluster tilesize and tilecount calculations.
Fixes #4127 & #3596

Solution

  • calculate tilesize as smallest integers such that dimensions.xy() tiles will cover the screen
  • calculate final dimensions as smallest integers such that final dimensions * tilesize will cover the screen

there is more cleanup that could be done in these functions. a future PR will likely remove the tilesize completely, so this is just a minimal change set to fix the current bug at small screen sizes

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 8, 2022
@robtfm robtfm mentioned this pull request Mar 8, 2022
1 task
@superdump superdump added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Testing A change that impacts how we test Bevy or how users test their apps and removed S-Needs-Triage This issue needs to be labelled labels Mar 8, 2022
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.

I think we should probably be using debug_assert for situations that could happen due to programming errors within bevy that would cause runtime problems, but that if we wrote our code properly, should never happen. And then use assert for things that could happen at runtime in release builds due to a user error, though proper error handling would be nicer than panicking... so I'm trying to think of a situation where assert should really be used... I realise I've used assert elsewhere in this code but the thought came up now. @cart @mockersf do you have any opinion / clear thoughts and guidelines on this?

crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
robtfm and others added 2 commits March 8, 2022 13:03
Co-authored-by: Robert Swain <robert.swain@gmail.com>
@cart
Copy link
Member

cart commented Mar 10, 2022

I think we should probably be using debug_assert for situations that could happen due to programming errors within bevy that would cause runtime problems

The rule of thumb I've been using:

  1. When possible, use tests instead of asserts. The more expensive an assertion is, the more you should try to make it a test. We shouldn't meaningfully compromise dev performance because we aren't confident in our code. For expensive checks, we should achieve confidence through other forms of testing.
  2. For cheap assertions that can't be tests, use debug_assert.

@cart
Copy link
Member

cart commented Mar 10, 2022

This particular case doesn't feel like a debug_assert. This is a user-facing type that has constraints on the values it supports. These constraints should either be enforced here (panic if unrecoverable, or warn + auto-constrain to avoid crashing) or they should be enforced via a constructor. I'm inclined to auto-constrain and warn here.

@cart
Copy link
Member

cart commented Mar 10, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 10, 2022
# Objective

fix cluster tilesize and tilecount calculations.
Fixes #4127 & #3596

## Solution

- calculate tilesize as smallest integers such that dimensions.xy() tiles will cover the screen
- calculate final dimensions as smallest integers such that final dimensions * tilesize will cover the screen

there is more cleanup that could be done in these functions. a future PR will likely remove the tilesize completely, so this is just a minimal change set to fix the current bug at small screen sizes

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title fix cluster tiling calculations [Merged by Bors] - fix cluster tiling calculations Mar 10, 2022
@bors bors bot closed this Mar 10, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective

fix cluster tilesize and tilecount calculations.
Fixes bevyengine#4127 & bevyengine#3596

## Solution

- calculate tilesize as smallest integers such that dimensions.xy() tiles will cover the screen
- calculate final dimensions as smallest integers such that final dimensions * tilesize will cover the screen

there is more cleanup that could be done in these functions. a future PR will likely remove the tilesize completely, so this is just a minimal change set to fix the current bug at small screen sizes

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

fix cluster tilesize and tilecount calculations.
Fixes bevyengine#4127 & bevyengine#3596

## Solution

- calculate tilesize as smallest integers such that dimensions.xy() tiles will cover the screen
- calculate final dimensions as smallest integers such that final dimensions * tilesize will cover the screen

there is more cleanup that could be done in these functions. a future PR will likely remove the tilesize completely, so this is just a minimal change set to fix the current bug at small screen sizes

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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-Bug An unexpected or incorrect behavior C-Testing A change that impacts how we test Bevy or how users test their apps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

render_to_texture example crashes with other texture sizes
3 participants