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] - Remove unnecessary alternate create_texture path in prepare_asset for Image #6671

Closed

Conversation

DGriffin91
Copy link
Contributor

Objective

prepare_asset for Image has an alternate path for texture creation that is used when the image is not compressed and does not contain mipmaps. This additional code path is unnecessary as render_device.create_texture_with_data() will handle both cases correctly.

Solution

Use render_device.create_texture_with_data() in all cases.

Tested successfully with the following examples:

  • load_gltf
  • render_to_texture
  • texture
  • 3d_shapes
  • sprite
  • sprite_sheet
  • array_texture
  • shader_material_screenspace_texture
  • skybox (though this already would use the create_texture_with_data() branch anyway)

Copy link
Contributor

@komadori komadori left a comment

Choose a reason for hiding this comment

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

All the parameters that went into write_texture() came from the texture descriptor, so it seems unlikely that it was handling any special case not covered by create_texture_with_data().

@mockersf
Copy link
Member

while this works the same, it is potentially slower in the no-mip/no-compression case as the code was taking a lot of shortcuts

@mockersf mockersf added the A-Rendering Drawing game state to the screen label Nov 18, 2022
@DGriffin91
Copy link
Contributor Author

DGriffin91 commented Nov 18, 2022

@mockersf
Benchmarked in vtune with 1 million 8x8px images.
Total time spent in prepare_asset() for Image:

Current bevy with branch:
Run 1: 21.299s
Run 2: 21.123s
Run 3: 21.305s

This PR:
Run 1: 19.939s
Run 2: 20.066s
Run 3: 20.596s

So this PR is apparently faster. It looks like at least part of this is because image.is_compressed() and image.texture_descriptor.format.pixel_size() are a bit slow.

@mockersf
Copy link
Member

nice! 🎉

@james7132 james7132 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Performance A change motivated by improving speed, memory usage or compile times labels Dec 3, 2022
@cart
Copy link
Member

cart commented Dec 5, 2022

bors r+

bors bot pushed a commit that referenced this pull request Dec 5, 2022
… Image (#6671)

# Objective

`prepare_asset` for Image has an alternate path for texture creation that is used when the image is not compressed and does not contain mipmaps. This additional code path is unnecessary as `render_device.create_texture_with_data()` will handle both cases correctly.

## Solution

Use `render_device.create_texture_with_data()` in all cases.

Tested successfully with the following examples:
- load_gltf
- render_to_texture
- texture
- 3d_shapes
- sprite
- sprite_sheet
- array_texture
- shader_material_screenspace_texture
- skybox (though this already would use the `create_texture_with_data()` branch anyway)
@bors bors bot changed the title Remove unnecessary alternate create_texture path in prepare_asset for Image [Merged by Bors] - Remove unnecessary alternate create_texture path in prepare_asset for Image Dec 5, 2022
@bors bors bot closed this Dec 5, 2022
@james7132 james7132 added this to the 0.10 milestone Dec 6, 2022
ickshonpe pushed a commit to ickshonpe/bevy that referenced this pull request Dec 6, 2022
… Image (bevyengine#6671)

# Objective

`prepare_asset` for Image has an alternate path for texture creation that is used when the image is not compressed and does not contain mipmaps. This additional code path is unnecessary as `render_device.create_texture_with_data()` will handle both cases correctly.

## Solution

Use `render_device.create_texture_with_data()` in all cases.

Tested successfully with the following examples:
- load_gltf
- render_to_texture
- texture
- 3d_shapes
- sprite
- sprite_sheet
- array_texture
- shader_material_screenspace_texture
- skybox (though this already would use the `create_texture_with_data()` branch anyway)
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
… Image (bevyengine#6671)

# Objective

`prepare_asset` for Image has an alternate path for texture creation that is used when the image is not compressed and does not contain mipmaps. This additional code path is unnecessary as `render_device.create_texture_with_data()` will handle both cases correctly.

## Solution

Use `render_device.create_texture_with_data()` in all cases.

Tested successfully with the following examples:
- load_gltf
- render_to_texture
- texture
- 3d_shapes
- sprite
- sprite_sheet
- array_texture
- shader_material_screenspace_texture
- skybox (though this already would use the `create_texture_with_data()` branch anyway)
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
… Image (bevyengine#6671)

# Objective

`prepare_asset` for Image has an alternate path for texture creation that is used when the image is not compressed and does not contain mipmaps. This additional code path is unnecessary as `render_device.create_texture_with_data()` will handle both cases correctly.

## Solution

Use `render_device.create_texture_with_data()` in all cases.

Tested successfully with the following examples:
- load_gltf
- render_to_texture
- texture
- 3d_shapes
- sprite
- sprite_sheet
- array_texture
- shader_material_screenspace_texture
- skybox (though this already would use the `create_texture_with_data()` branch anyway)
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-Performance A change motivated by improving speed, memory usage or compile times 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.

5 participants