Skip to content

Conversation

@rparrett
Copy link
Contributor

@rparrett rparrett commented Dec 11, 2024

Objective

Fixes #12359

Solution

Implement alternative number 4.

#12359 (comment)

I don't think that I agree with the premise of this issue anymore. I am not sure that entities "magically" despawning themselves or components removing themselves make for great defaults in an "ECS-based API". This behavior is likely to be just as surprising to people.

I think that the lack of sink re-usability should be treated as a bug and possibly the documentation improved to reflect the current limitations if it doesn't seem like a fix is forthcoming.
-- me

@rparrett rparrett changed the title Remove audio TODO and add docs about limitations of PlaybackMode::Once Remove TODO and add docs about limitations of PlaybackMode::Once Dec 11, 2024
@alice-i-cecile alice-i-cecile added A-Audio Sounds playback and modification C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through labels Dec 11, 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.

Good docs: this is definitely better than nothing. I think we should remove Once completely and default to Remove. If you can't reuse this and need to replace the component anyways, it doesn't really seem like there's a point.

@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Dec 11, 2024
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

I imagine that this documentation will be updated when the missing feature "bug" is implemented.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 12, 2024
@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 Dec 12, 2024
Merged via the queue into bevyengine:main with commit 33a1a55 Dec 12, 2024
33 checks passed
Trashtalk217 pushed a commit to Trashtalk217/bevy that referenced this pull request Dec 21, 2024
…evyengine#16769)

# Objective

Fixes bevyengine#12359

## Solution

Implement alternative number 4.

bevyengine#12359 (comment)
> I don't think that I agree with the premise of this issue anymore. I
am not sure that entities "magically" despawning themselves or
components removing themselves make for great defaults in an "ECS-based
API". This behavior is likely to be just as surprising to people.
>
> I think that the lack of sink re-usability should be treated as a bug
and possibly the documentation improved to reflect the current
limitations if it doesn't seem like a fix is forthcoming.
> -- me
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
…evyengine#16769)

# Objective

Fixes bevyengine#12359

## Solution

Implement alternative number 4.

bevyengine#12359 (comment)
> I don't think that I agree with the premise of this issue anymore. I
am not sure that entities "magically" despawning themselves or
components removing themselves make for great defaults in an "ECS-based
API". This behavior is likely to be just as surprising to people.
>
> I think that the lack of sink re-usability should be treated as a bug
and possibly the documentation improved to reflect the current
limitations if it doesn't seem like a fix is forthcoming.
> -- me
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Audio Sounds playback and modification 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 X-Contentious There are nontrivial implications that should be thought through

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Consider making PlaybackSettings::ONCE not the default or removing it

3 participants