Fix underflow panic in InitTriInfo

[kumikaya]

Objective

• Fix GenerateTangents in 0.14.1 can panic even though it is supposed to return Result  #14874

Solution

• Change the place where a panic occurs from t < iNrTrianglesIn - 1 to t + 1 < iNrTrianglesIn.

Testing

• After the fix, the following code runs successfully without any panic.

use bevy::prelude::Mesh;
use bevy_render::{
    mesh::{Indices, PrimitiveTopology},
    render_asset::RenderAssetUsages,
};

const POSITIONS: &[[f32; 3]] = &[[0.0, 1.0, 0.0], [0.0, 0.0, 0.0], [0.0, 1.0, 0.0]];

const NORMALS: &[[f32; 3]] = &[[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]];

const INDICES: &[u32] = &[0, 1, 2];

const UVS: &[[f32; 2]] = &[[0.0, 1.0], [0.0, 0.0], [0.0, 1.0]];

fn main() {
    let mut mesh = Mesh::new(
        PrimitiveTopology::TriangleList,
        RenderAssetUsages::default(),
    );

    mesh.insert_attribute(Mesh::ATTRIBUTE_POSITION, POSITIONS.to_vec());
    mesh.insert_attribute(Mesh::ATTRIBUTE_UV_0, UVS.to_vec());
    mesh.insert_attribute(Mesh::ATTRIBUTE_NORMAL, NORMALS.to_vec());
    mesh.insert_indices(Indices::U32(INDICES.to_vec()));
    mesh.generate_tangents().ok();

}

Migration Guide

• No breaking changes introduced.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[janhohenheim]

Note that this will add a merge conflict to #9050

[alice-i-cecile]

@ethereumdegen your review here would be appreciated.

[janhohenheim]

I'm honestly not convinced that the code in question should run smoothly. That mesh seems whacky and bound to cause problems down the line. @ethereumdegen's offending mesh was similarly malformed. Do we at least print a warn! in this case?

[ethereumdegen]

I'm honestly not convinced that the code in question should run smoothly. That mesh seems whacky and bound to cause problems down the line. @ethereumdegen's offending mesh was similarly malformed. Do we at least print a warn! in this case?

It doesnt have to run smoothly it can throw an error from the result

But it shouldnt crash the entire program

[janhohenheim]

Mesh::new doesn't return a result. Am I missing something?
Agreed that ideally we want to return an error somewhere instead of panicking, but it seems to me like this PR instead changes the behavior to instead silently pretend everything is alright.

[ethereumdegen]

Yeah my pr is about generate_tangents not mesh new

…

On Fri, Aug 23, 2024 at 12:57 PM Jan Hohenheim ***@***.***> wrote: Mesh::new doesn't return a result. Am I missing something? — Reply to this email directly, view it on GitHub <#14893 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPVWL5TC4ZJQW3TZYDS7OTZS5SXXAVCNFSM6AAAAABNAJSJFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBXGQ3DQMRSHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[janhohenheim]

@ethereumdegen Ooooooh I see, I didn't scroll down enough on the example code, haha. That makes sense then :)
Then my issue becomes that the code posted above seems to not return an error on generate_tangents. I didn't run it, so I might be misunderstanding the behavior @kumikaya

[kumikaya]

Maybe it would be more appropriate to change the place where a panic occurs from t < iNrTrianglesIn - 1 to t + 1 < iNrTrianglesIn.

[janhohenheim]

@kumikaya I would prefer it if there was no panic at all, and instead generate_tangents returned an error in this case.
Also, no need to force push your commits to clean the history; the PR get squashed anyways and by leaving the history intact you make it easier for future readers :)

[kumikaya]

@janhohenheim Yeah, it would be better to check if iNrTrianglesIn is zero and return false after iNrTrianglesIn = iTotTris - iDegenTriangles, but I'm not quite sure at which point it would be best to return; thanks for your advice.

[janhohenheim]

@kumikaya feel free to ping me when the code you posted in the PR description results in generate_tangents returning an Err variant :)

[kumikaya]

@janhohenheim I've updated the code in the PR,genTangSpace returns false immediately when it detects that the number of valid triangles is 0. This causes generate_tangents, which indirectly calls genTangSpace, to return an Err variant.

[janhohenheim]

Looks good to me then :)
Thanks for fixing the crash!