spnda / fastgltf

A modern C++17 glTF 2.0 library focused on speed, correctness, and usability
https://fastgltf.readthedocs.io/v0.8.x/
MIT License
301 stars 44 forks source link

SEGV when iterating .gltf primitive indicied accessor via iterateAccessor or iterateAccessorWithIndex #45

Closed morgankdavis closed 9 months ago

morgankdavis commented 9 months ago

See the attached test scene. pineapple_only.zip

Load and parse the scene and try to iterate over the first primitive's indicesAccessor using iterateAccessor() or iterateAccessorWithIndex(). For example:

` fastgltf::Asset& asset // exists fastgltf::Primitive& primitive // exists

if (auto indiciesAccessorIndex = primitive.indicesAccessor) {

    vector<uint32_t> indices;
    if (primitive.indicesAccessor.has_value()) {
        auto &accessor = asset.accessors[*indiciesAccessorIndex];
        indices.resize(accessor.count);

        iterateAccessorWithIndex<uint32_t>(
                asset, accessor, [&](uint32_t index, size_t idx) {
                    indices[idx] = index;
                });
    }
}

`

Observe SEGV at getAccessorElementAt(), case ComponentType::UnsignedShort.

Thanks! Awesome library!

spnda commented 9 months ago

A SEGV in getAccessorElement leads me to believe the buffers aren't loaded and the code then tries to dereference a nullptr. Are you positive you either load buffers yourself or specify Options::LoadGLBBuffers and/or Options::LoadExternalBuffers?

Otherwise, do you have any more specific information when the exception occurs?

morgankdavis commented 9 months ago

Failing to specify any options was the issue; that also explains problems I was having loading texture data. Thank you.

spnda commented 9 months ago

I thought about this a bit over night. Would it perhaps make sense to add some sort of assert in debug mode that would have a message such as "Buffers are not loaded but required for the accessor tools" so that newer users who missed that part of the documentation that buffers and images aren't loaded by default would understand what the issue is much quicker? Or do you have any suggestions how to make this a bit more user friendly?

morgankdavis commented 9 months ago

I think an assert would be appropriate.