Skip to content

add native zstd support #19793

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
merged 5 commits into from
Jun 26, 2025
Merged

add native zstd support #19793

merged 5 commits into from
Jun 26, 2025

Conversation

atlv24
Copy link
Contributor

@atlv24 atlv24 commented Jun 24, 2025

Objective

  • add support for alternate zstd backend through zstd for faster decompression

Solution

  • make existing zstd feature only specify that support is required, disambiguate which backend to use via two other features zstd_native and zstd_rust.
  • Similar to the approach taken by KTX2 Updates: ETC1s/BasisLZ, ASTC HDR, and faster Zstd #18411, but we keep current behavior by defaulting to the rust implementation because its safer, and isolate this change.

NOTE: the default feature-set may seem to not currently require zstd, but it does, it is enabled transitively by the tonemapping_luts feature, which is a default feature. Thus this does not add default features.

Testing

  • Cargo clippy on both feature combinations

@atlv24 atlv24 added A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 24, 2025
@Victoronz Victoronz added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 24, 2025
@mnmaita
Copy link
Member

mnmaita commented Jun 24, 2025

Should this have a small migration guide entry to state that an additional feature is required when using zstd?

@atlv24
Copy link
Contributor Author

atlv24 commented Jun 24, 2025

its a default feature, so unless you're using default-features=false there's no migration

@atlv24 atlv24 force-pushed the zstd branch 2 times, most recently from b48013b to 7c2fb6d Compare June 24, 2025 10:22
@janhohenheim
Copy link
Member

janhohenheim commented Jun 24, 2025

@atlv24 doing default-features = false is the recommended way of creating third-party plugins, so lots of people use that.
As a plugin author, I'd be happy if we considered things like this in our migration guide. I've many times upgraded a plugin to a new Bevy version only to get cryptic error messages because some feature is now needed, but not documented anywhere ;)

@mockersf
Copy link
Member

it's mentioned in the migration guide 👍

@janhohenheim
Copy link
Member

Oh thanks, then I'm happy :D

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 26, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jun 26, 2025
# Objective

- add support for alternate zstd backend through `zstd` for faster
decompression

## Solution

- make existing `zstd` feature only specify that support is required,
disambiguate which backend to use via two other features `zstd_native`
and `zstd_rust`.
- Similar to the approach taken by #18411, but we keep current behavior
by defaulting to the rust implementation because its safer, and isolate
this change.

NOTE: the default feature-set may seem to not currently require `zstd`,
but it does, it is enabled transitively by the `tonemapping_luts`
feature, which is a default feature. Thus this does not add default
features.

## Testing

- Cargo clippy on both feature combinations
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 26, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 26, 2025
Merged via the queue into bevyengine:main with commit 3080040 Jun 26, 2025
36 checks passed
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 D-Straightforward Simple bug fixes and API improvements, docs, test and 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
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants