Skip to content

Conversation

@bushrat011899
Copy link
Contributor

Objective

Solution

Generalised From/Into implementations over &str and Option<&str> for AssetSourceId and AssetPath across all lifetimes, not just static. To maintain access to the 'static-only specialisation, these types (and CowArc) now include an as_static method which will apply the specialisation.

// Snipped from `AssetApp`
fn register_asset_source(
    &mut self,
    id: impl Into<AssetSourceId<'static>>,
    //                          ^^^^^^^
    //                          | as_static is only available for 'static lifetimes
    source: AssetSourceBuilder,
) -> &mut Self {
    let id = id.into().as_static();
    //          ^^^^^^ ^^^^^^^^^
    //          |      | Specialized (internally storing CowArc::Static)
    //          | Generic Into (internally storing CowArc::Borrowed)
    
    // ...
}

This post-fix specialisation is available here because the actual specialisation performed is only a marker for if/when modification or ownership is required, making the transform a very cheap operation. For cleanliness, I've also added from_static, which wraps this behaviour in a clean shorthand designed to replace from calls.


Changelog

  • Generalised the following implementations over a generic lifetime:
    • From<&'static str> for AssetSourceId<'static>
    • From<Option<&'static str>> for AssetSourceId<'static>
    • From<&'static str> for AssetPath<'static>
    • From<&'static Path> for AssetPath<'static>
  • Added as_static specialisation to:
    • CowArc
    • AssetSourceId
    • AssetPath
  • Added from_static specialised constructor to:
    • AssetSourceId
    • AssetPath

Migration Guide

In areas where these implementations where being used, you can now add from_static in order to get the original specialised implementation which avoids creating an Arc internally.

// Before
let asset_path = AssetPath::from("my/path/to/an/asset.ext");

// After
let asset_path = AssetPath::from_static("my/path/to/an/asset.ext");

To be clear, this is only required if you wish to maintain the performance benefit that came with the specialisation. Existing code is not broken by this change.

@Nilirad Nilirad added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 1, 2023
@rparrett rparrett 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 Aug 13, 2024
@alice-i-cecile
Copy link
Member

Merging once merge conflicts are resolved :)

Allows creating `AssetSourceId` and `AssetPath` from non-static lifetimes using the `From/Into` implementations over `&str` and `Option<&str>`.

Added `as_static` and `from_static` methods to allow a work-around specialization.
@bushrat011899 bushrat011899 force-pushed the AssetPathAndSourceIDIntoGenericLifetime branch from 773e844 to 5068b6a Compare August 19, 2024 23:31
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 19, 2024
Merged via the queue into bevyengine:main with commit 491aec8 Aug 19, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 15, 2025
…in AssetServer::load() (#20562)

# Objective

- Fixes #19844

## Solution

- Revert #10823
- Also reverts #19094, as the ignored test added there does not compile
with the new lifetime rules.

## TODO

This PR was made > 10k issues ago, and git can do funky things. Opening
as a draft to investigate the diff before proceeding.

- [x] merge main into the old branch
- [x] revert the PR 
- [x] make sure the diff looks okay
- [x] add comments explaining why we did this weird thing

---------

Co-authored-by: Zac Harrold <zac@harrold.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

Restrictive AssetPath From impls cause hard-to-diagnose compile errors

6 participants