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

Add feature requirement info to image loading docs #13712

Conversation

bugsweeper
Copy link
Contributor

@bugsweeper bugsweeper commented Jun 6, 2024

Objective

Solution

  • Added #[cfg(feature = "")] for types and warn!("feature "" is not enabled"); for ImageFormat enum conversions

Testing

ran reproducing example from issue #13468 and saw in logs
WARN bevy_render::texture::image: feature "exr" is not enabled

generated docs with command RUSTDOCFLAGS="-Zunstable-options --cfg=docsrs" cargo +nightly doc --workspace --all-features --no-deps --document-private-items --open and saw
image
that docs contain Available on crate feature <image format> only. marks
image

Migration Guide

Image format related entities are feature gated, if there are compilation errors about unknown names there are some of features in list (exr, hdr, basis-universal, png, dds, tga, jpeg, bmp, ktx2, webp and pnm) should be added.

@bugsweeper bugsweeper force-pushed the add_feature_requirement_info_to_image_loading_docs branch 2 times, most recently from 48a1791 to 842a06e Compare June 6, 2024 13:00
@bugsweeper bugsweeper force-pushed the add_feature_requirement_info_to_image_loading_docs branch from 842a06e to 4cc0c0f Compare June 6, 2024 13:03
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen labels Jun 6, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I generally really like it! Shouldn't we use module-level feature gates for e.g. ktx.rs?

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon labels Jun 6, 2024
@BD103 BD103 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 6, 2024
Copy link
Contributor

github-actions bot commented Jun 6, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@BD103
Copy link
Member

BD103 commented Jun 6, 2024

I added the breaking change label because some enum variants of ImageFormat may no longer be available due to feature gates. (The same may be true for the modules, I'm not sure.)

@bugsweeper
Copy link
Contributor Author

Shouldn't we use module-level feature gates for e.g. ktx.rs?

ktx2 module already has feature gate, but rust compiler isn't smart enough to mark all inner items of module with feature mark, that's why I marked explicitly

@alice-i-cecile
Copy link
Member

Oh? That's surprising to me. What happens if it's ommitted? Do you have any references to bugs etc for this?

@bugsweeper
Copy link
Contributor Author

What happens if it's ommitted?

Before my commit there already was attribute on ktx2 module and functions were not marked in docs. Now if attribute on module exists, but for function it was removed, then there is no mark in doc, if attribute on function exists, but for module it was removed, then mark in doc for function exists. I assumed that higher level attribute could reduce binary size, that is why i kept them both.

Do you have any references to bugs etc for this?

I didn't see any related issue earlier, and I didn't find it now. I just reacted to compiller behaviour. If I made an incorrect changes, I'm open to any corrections.

@alice-i-cecile
Copy link
Member

Sounds good :) I'll file something for the rustdoc team reporting this!

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Not sure I understand why logging versus a compiler error is preferred given this seems to be a bug but this is certainly an improvement in user feedback either way.

@tychedelia tychedelia 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 Aug 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 16, 2024
Merged via the queue into bevyengine:main with commit bc445bb Aug 16, 2024
35 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 C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Image loading docs are not clear enough about required feature flags
4 participants