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

Only use the AABB center for mesh visibility range testing if specified. #16468

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

pcwalton
Copy link
Contributor

PR #15164 made Bevy consider the center of the mesh to be the center of the axis-aligned bounding box (AABB). Unfortunately, this breaks crossfading in many cases. LODs may have different AABBs and so the center of the AABB may differ for different LODs of the same mesh. The crossfading, however, relies on all LODs having precisely the same position.

To address this problem, this PR adds a new field, use_aabb, to VisibilityRange, which makes the AABB center point behavior opt-in.

@BenjaminBrienen first noticed this issue when reviewing PR #16286. That PR contains a video showing the effects of this regression on the visibility_range example. This commit fixes that example.

Migration Guide

  • The VisibilityRange component now has an extra field, use_aabb. Generally, you can safely set it to false.

PR bevyengine#15164 made Bevy consider the center of the mesh to be the center of
the axis-aligned bounding box (AABB). Unfortunately, this breaks
crossfading in many cases. LODs may have different AABBs and so the
center of the AABB may differ for different LODs of the same mesh. The
crossfading, however, relies on all LODs having *precisely* the same
position.

To address this problem, this PR adds a new field, `use_aabb`, to
`VisibilityRange`, which makes the AABB center point behavior opt-in.

@BenjaminBrienen first noticed this issue when reviewing PR bevyengine#16286. That
PR contains a video showing the effects of this regression on the
`visibility_range` example. This commit fixes that example.
@pcwalton pcwalton requested a review from IceSentry November 22, 2024 06:23
@pcwalton pcwalton added A-Rendering Drawing game state to the screen P-Regression Functionality that used to work but no longer does. Add a test for this! C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 22, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 22, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Nov 22, 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 Nov 22, 2024
@alice-i-cecile
Copy link
Member

Great docs!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 22, 2024
Merged via the queue into bevyengine:main with commit d80b809 Nov 22, 2024
31 checks passed
@pcwalton pcwalton deleted the visibility-range-aabb branch November 22, 2024 19:31
mockersf pushed a commit that referenced this pull request Nov 22, 2024
…ed. (#16468)

PR #15164 made Bevy consider the center of the mesh to be the center of
the axis-aligned bounding box (AABB). Unfortunately, this breaks
crossfading in many cases. LODs may have different AABBs and so the
center of the AABB may differ for different LODs of the same mesh. The
crossfading, however, relies on all LODs having *precisely* the same
position.

To address this problem, this PR adds a new field, `use_aabb`, to
`VisibilityRange`, which makes the AABB center point behavior opt-in.

@BenjaminBrienen first noticed this issue when reviewing PR #16286. That
PR contains a video showing the effects of this regression on the
`visibility_range` example. This commit fixes that example.

## Migration Guide

* The `VisibilityRange` component now has an extra field, `use_aabb`.
Generally, you can safely set it to false.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Dec 2, 2024
…ed. (bevyengine#16468)

PR bevyengine#15164 made Bevy consider the center of the mesh to be the center of
the axis-aligned bounding box (AABB). Unfortunately, this breaks
crossfading in many cases. LODs may have different AABBs and so the
center of the AABB may differ for different LODs of the same mesh. The
crossfading, however, relies on all LODs having *precisely* the same
position.

To address this problem, this PR adds a new field, `use_aabb`, to
`VisibilityRange`, which makes the AABB center point behavior opt-in.

@BenjaminBrienen first noticed this issue when reviewing PR bevyengine#16286. That
PR contains a video showing the effects of this regression on the
`visibility_range` example. This commit fixes that example.

## Migration Guide

* The `VisibilityRange` component now has an extra field, `use_aabb`.
Generally, you can safely set it to false.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
…ed. (bevyengine#16468)

PR bevyengine#15164 made Bevy consider the center of the mesh to be the center of
the axis-aligned bounding box (AABB). Unfortunately, this breaks
crossfading in many cases. LODs may have different AABBs and so the
center of the AABB may differ for different LODs of the same mesh. The
crossfading, however, relies on all LODs having *precisely* the same
position.

To address this problem, this PR adds a new field, `use_aabb`, to
`VisibilityRange`, which makes the AABB center point behavior opt-in.

@BenjaminBrienen first noticed this issue when reviewing PR bevyengine#16286. That
PR contains a video showing the effects of this regression on the
`visibility_range` example. This commit fixes that example.

## Migration Guide

* The `VisibilityRange` component now has an extra field, `use_aabb`.
Generally, you can safely set it to false.
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-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants