Skip to content

Revert asset lifetime changes and preserve staticness of asset paths in AssetServer::load() #20562

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.

Sign up for GitHub

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

Jump to bottom
Merged
alice-i-cecile merged 6 commits into from
Aug 15, 2025
Merged

Revert asset lifetime changes and preserve staticness of asset paths in AssetServer::load() #20562

alice-i-cecile merged 6 commits into from
Aug 15, 2025

Conversation

@alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Aug 13, 2025 •
edited
Loading

Objective

Solution

TODO

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

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

bushrat011899 and others added 3 commits August 20, 2024 09:28
@bushrat011899
Generalized Into<AssetSourceId> and Into<AssetPath> Implementations
5068b6a 
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.
@alice-i-cecile
Merge branch 'main' into AssetPathAndSourceIDIntoGenericLifetime
fafcf49
@alice-i-cecile
Revert "Generalized Into<AssetSourceId> and Into<AssetPath> Imple…
a3370fc 
…mentations"

This reverts commit 5068b6a.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times P-Regression Functionality that used to work but no longer does. Add a test for this! labels Aug 13, 2025
alice-i-cecile
alice-i-cecile commented Aug 13, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile commented Aug 13, 2025
View reviewed changes
@alice-i-cecile alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 13, 2025
@alice-i-cecile alice-i-cecile marked this pull request as ready for review August 13, 2025 19:53
@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 and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 14, 2025
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Aug 14, 2025
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Aug 14, 2025

Hmm. Fixing that does not look fun. I'll tackle it tomorrow.

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Aug 14, 2025
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Aug 15, 2025 •
edited
Loading

The CI failure was in a test from #19094, which is ignored, and blocked on #11111. Fixing lifetime issues with an ignored test that does not work is not a good use of time. Removing.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 15, 2025
andriyDev
andriyDev approved these changes Aug 15, 2025
View reviewed changes
Copy link
Contributor

@andriyDev andriyDev 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 fix up the ignored test eventually but I don't think this PR needs to figure that out.

@alice-i-cecile alice-i-cecile 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 15, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 15, 2025
Merged via the queue into bevyengine:main with commit 3d89327 Aug 15, 2025
36 checks passed
andriyDev added a commit to andriyDev/bevy that referenced this pull request Oct 16, 2025
@andriyDev
Revive test from bevyengine#19094, removed in bevyengine#20562.
b8c5712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cart cart Awaiting requested review from cart

@bushrat011899 bushrat011899 Awaiting requested review from bushrat011899

@atlv24 atlv24 Awaiting requested review from atlv24

@james7132 james7132 Awaiting requested review from james7132

1 more reviewer

@andriyDev andriyDev andriyDev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes P-Regression Functionality that used to work but no longer does. Add a test for this! 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

Milestone

0.17

Development

Successfully merging this pull request may close these issues.

Static-ness is not conserved for asset paths in AssetServer::load()

5 participants

@alice-i-cecile @james7132 @atlv24 @andriyDev @bushrat011899