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.

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

Revert "Support calculating normals for indexed meshes" #12716

Closed
alice-i-cecile opened this issue Mar 25, 2024 · 1 comment · Fixed by #13333
Closed

Revert "Support calculating normals for indexed meshes" #12716

alice-i-cecile opened this issue Mar 25, 2024 · 1 comment · Fixed by #13333
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy
Milestone
0.14

Comments

@alice-i-cecile
Copy link
Member

This PR should probably be reverted. LIke I said in the original PR using compute_flat_normals() here is misleading because it's not computing flat normals. We should have a separate compute_smooth_normals() function for this.

Originally posted by @IceSentry in #11654 (comment)
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen D-Trivial Nice and easy! A great choice to get started with Bevy labels Mar 25, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Mar 25, 2024
@bugsweeper
Copy link
Contributor

Is this supposed to have separate compute_flat_normals() and compute_smooth_normals() using one of them only for non indexed meshes and using second one only for indexed meshes (should they panic otherwise)? Wouldn't be better to have single compute_normals(), which works well in both cases automaticaly?

adithramachandran added a commit to adithramachandran/bevy-fork-dev that referenced this issue May 12, 2024
@adithramachandran
Revert "Support calculating normals for indexed meshes" (bevyengine#1…
ea98b53 
…2716)

# Objective

- Refactor the changes merged in bevyengine#11654 to compute flat normals for
indexed meshes instead of smooth normals.

## Solution

- Partially revert the changes in bevyengine#11654 to compute flat normals for
both indexed and unindexed meshes in `compute_flat_normals`
- Create a new method, `compute_smooth_normals`, that computes smooth
normals for indexed meshes
- Create a new method, `compute_normals`, that computes smooth normals
for indexed meshes and flat normals for unindexed meshes by default. Use
this new method instead of `compute_flat_normals`.

## Testing

- Run the example with and without the changes to ensure that the
results are identical.
@adithramachandran adithramachandran mentioned this issue May 12, 2024
Merged
adithramachandran added a commit to adithramachandran/bevy-fork-dev that referenced this issue May 12, 2024
@adithramachandran
Revert "Support calculating normals for indexed meshes" (bevyengine#1…
532728c 
…2716)

# Objective

- Refactor the changes merged in bevyengine#11654 to compute flat normals for
indexed meshes instead of smooth normals.

## Solution

- Partially revert the changes in bevyengine#11654 to compute flat normals for
both indexed and unindexed meshes in `compute_flat_normals`
- Create a new method, `compute_smooth_normals`, that computes smooth
normals for indexed meshes
- Create a new method, `compute_normals`, that computes smooth normals
for indexed meshes and flat normals for unindexed meshes by default. Use
this new method instead of `compute_flat_normals`.

## Testing

- Run the example with and without the changes to ensure that the
results are identical.
adithramachandran added a commit to adithramachandran/bevy-fork-dev that referenced this issue May 12, 2024
@adithramachandran
Revert "Support calculating normals for indexed meshes" (bevyengine#1…
d2c912e 
…2716)

# Objective

- Refactor the changes merged in bevyengine#11654 to compute flat normals for
indexed meshes instead of smooth normals.

## Solution

- Partially revert the changes in bevyengine#11654 to compute flat normals for
both indexed and unindexed meshes in `compute_flat_normals`
- Create a new method, `compute_smooth_normals`, that computes smooth
normals for indexed meshes
- Create a new method, `compute_normals`, that computes smooth normals
for indexed meshes and flat normals for unindexed meshes by default. Use
this new method instead of `compute_flat_normals`.

## Testing

- Run the example with and without the changes to ensure that the
results are identical.
adithramachandran added a commit to adithramachandran/bevy-fork-dev that referenced this issue May 14, 2024
@adithramachandran
Revert "Support calculating normals for indexed meshes" (bevyengine#1…
968f051 
…2716)

# Objective

- Refactor the changes merged in bevyengine#11654 to compute flat normals for
indexed meshes instead of smooth normals.

## Solution

- Partially revert the changes in bevyengine#11654 to compute flat normals for
both indexed and unindexed meshes in `compute_flat_normals`
- Create a new method, `compute_smooth_normals`, that computes smooth
normals for indexed meshes
- Create a new method, `compute_normals`, that computes smooth normals
for indexed meshes and flat normals for unindexed meshes by default. Use
this new method instead of `compute_flat_normals`.

## Testing

- Run the example with and without the changes to ensure that the
results are identical.
github-merge-queue bot pushed a commit that referenced this issue May 16, 2024
@adithramachandran
Revert "Support calculating normals for indexed meshes" (#12716) and …
65e62ba 
…add support for calculating smooth normals (#13333)

# Objective

- Refactor the changes merged in #11654 to compute flat normals for
indexed meshes instead of smooth normals.
- Fixes #12716 

## Solution

- Partially revert the changes in #11654 to compute flat normals for
both indexed and unindexed meshes in `compute_flat_normals`
- Create a new method, `compute_smooth_normals`, that computes smooth
normals for indexed meshes
- Create a new method, `compute_normals`, that computes smooth normals
for indexed meshes and flat normals for unindexed meshes by default. Use
this new method instead of `compute_flat_normals`.

## Testing

- Run the example with and without the changes to ensure that the
results are identical.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Milestone
0.14
2 participants
@alice-i-cecile @bugsweeper