Skip to content

Commit

Permalink
Fix: Rework Category enum due to parsing bug
Browse files Browse the repository at this point in the history 
Because for example the Scenes enumeration also had the Nodes bit set, parsing scenes would also (accidentally) tell the parser that the nodes were parsed, even though they weren't.
  • Loading branch information
@spnda
spnda committed Mar 13, 2023
1 parent b13df8f commit a334d38
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 37 deletions.
32 changes: 32 additions & 0 deletions src/fastgltf.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,35 @@ std::pair<fg::Error, fg::DataSource> fg::glTF::decodeUri(std::string_view uri) c
}
}

void fg::glTF::fillCategories(Category& inputCategories) noexcept {
if (inputCategories == Category::All)
return;

// The Category enum used to already OR values together so that e.g. Scenes would also implicitly
// have the Nodes bit set. This, however, caused some issues within the parse function as it tries
// to bail out when all requested categories have been parsed, as now something that hasn't been
// parsed could still be set. So, this has to exist...
if (hasBit(inputCategories, Category::Scenes))
inputCategories |= Category::Nodes;
if (hasBit(inputCategories, Category::Nodes))
inputCategories |= Category::Cameras | Category::Meshes | Category::Skins;
if (hasBit(inputCategories, Category::Skins))
// Skins needs nodes, nodes needs skins. To counter this circular dep we just redefine what we just wrote above.
inputCategories |= Category::Accessors | (Category::Nodes | Category::Cameras | Category::Meshes | Category::Skins);
if (hasBit(inputCategories, Category::Meshes))
inputCategories |= Category::Accessors | Category::Materials;
if (hasBit(inputCategories, Category::Materials))
inputCategories |= Category::Textures;
if (hasBit(inputCategories, Category::Animations))
inputCategories |= Category::Accessors;
if (hasBit(inputCategories, Category::Textures))
inputCategories |= Category::Images | Category::Samplers;
if (hasBit(inputCategories, Category::Images) || hasBit(inputCategories, Category::Accessors))
inputCategories |= Category::BufferViews;
if (hasBit(inputCategories, Category::BufferViews))
inputCategories |= Category::Buffers;
}

fg::MimeType fg::glTF::getMimeTypeFromString(std::string_view mime) {
if (mime == mimeTypeJpeg) {
return MimeType::JPEG;
Expand Down Expand Up @@ -574,6 +603,7 @@ fg::Error fg::glTF::validate() {

fg::Error fg::glTF::parse(Category categories) {
using namespace simdjson;
fillCategories(categories);

if (!hasBit(options, Options::DontRequireValidAssetMember)) {
dom::object asset;
Expand Down Expand Up @@ -697,6 +727,8 @@ fg::Error fg::glTF::parse(Category categories) {
#undef KEY_SWITCH_CASE
}

parsedAsset->availableCategories = readCategories;

return errorCode;
}

Expand Down
28 changes: 1 addition & 27 deletions src/fastgltf_parser.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,33 +163,6 @@ namespace fastgltf {
FASTGLTF_ASSIGNMENT_OP_TEMPLATE_MACRO(Options, Options, |)
FASTGLTF_ASSIGNMENT_OP_TEMPLATE_MACRO(Options, Options, &)

// clang-format off
enum class Category : uint32_t {
None = 0,
Buffers = 1 << 0,
BufferViews = 1 << 1 | Buffers,
Accessors = 1 << 2 | BufferViews,
Images = 1 << 3 | BufferViews,
Samplers = 1 << 4,
Textures = 1 << 5 | Images | Samplers,
Animations = 1 << 6 | Accessors,
Cameras = 1 << 7,
Materials = 1 << 8 | Textures,
Meshes = 1 << 9 | Accessors | Materials,
Skins = 1 << 10 | Accessors | (1 << 11), // Also depends on Nodes
Nodes = 1 << 11 | Cameras | Meshes | Skins,
Scenes = 1 << 12 | Nodes,
Asset = 1 << 13,

All = Asset | Scenes | Animations,
};
// clang-format on

FASTGLTF_ARITHMETIC_OP_TEMPLATE_MACRO(Category, Category, |)
FASTGLTF_ARITHMETIC_OP_TEMPLATE_MACRO(Category, Category, &)
FASTGLTF_ASSIGNMENT_OP_TEMPLATE_MACRO(Category, Category, |)
FASTGLTF_ASSIGNMENT_OP_TEMPLATE_MACRO(Category, Category, &)

// String representations of glTF 2.0 extension identifiers.
namespace extensions {
constexpr std::string_view EXT_mesh_gpu_instancing = "EXT_mesh_gpu_instancing";
Expand Down Expand Up @@ -224,6 +197,7 @@ namespace fastgltf {
explicit glTF(std::unique_ptr<ParserData> data, std::filesystem::path directory, Options options);

static auto getMimeTypeFromString(std::string_view mime) -> MimeType;
static void fillCategories(Category& inputCategories) noexcept;

[[nodiscard]] auto decodeUri(std::string_view uri) const noexcept -> std::pair<Error, DataSource>;
[[gnu::always_inline]] inline Error parseTextureObject(void* object, std::string_view key, TextureInfo* info) noexcept;
Expand Down
31 changes: 31 additions & 0 deletions src/fastgltf_types.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,34 @@ namespace fastgltf {
Spot,
Point,
};

enum class Category : uint32_t {
None = 0,
Buffers = 1 << 0,
BufferViews = 1 << 1,
Accessors = 1 << 2,
Images = 1 << 3,
Samplers = 1 << 4,
Textures = 1 << 5,
Animations = 1 << 6,
Cameras = 1 << 7,
Materials = 1 << 8,
Meshes = 1 << 9,
Skins = 1 << 10,
Nodes = 1 << 11,
Scenes = 1 << 12,
Asset = 1 << 13,

All = ~(~0u << 14),
// Includes everything needed for rendering but animations
OnlyRenderable = All & ~(Animations) & ~(Skins),
OnlyAnimations = Animations | Accessors | BufferViews | Buffers,
};

FASTGLTF_ARITHMETIC_OP_TEMPLATE_MACRO(Category, Category, |)
FASTGLTF_ARITHMETIC_OP_TEMPLATE_MACRO(Category, Category, &)
FASTGLTF_ASSIGNMENT_OP_TEMPLATE_MACRO(Category, Category, |)
FASTGLTF_ASSIGNMENT_OP_TEMPLATE_MACRO(Category, Category, &)
// clang-format on
#pragma endregion

Expand Down Expand Up @@ -924,6 +952,9 @@ namespace fastgltf {
std::vector<Skin> skins;
std::vector<Texture> textures;

// Keeps tracked of categories that were actually parsed.
Category availableCategories = Category::None;

explicit Asset() = default;
explicit Asset(const Asset& scene) = delete;
Asset& operator=(const Asset& scene) = delete;
Expand Down
2 changes: 1 addition & 1 deletion tests/base64_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ TEST_CASE("Check big base64 data decoding", "[base64]") {
REQUIRE(output.is_open());
std::vector<uint8_t> decodedBytes(output.tellg());
output.seekg(0);
output.read(reinterpret_cast<char*>(decodedBytes.data()), decodedBytes.size());
output.read(reinterpret_cast<char*>(decodedBytes.data()), static_cast<std::streamsize>(decodedBytes.size()));

REQUIRE(bytes == decodedBytes);
}
Expand Down
18 changes: 9 additions & 9 deletions tests/basic_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ TEST_CASE("Loading some basic glTF", "[gltf-loader]") {
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(cubeGltf != nullptr);

REQUIRE(cubeGltf->parse(fastgltf::Category::Scenes) == fastgltf::Error::None);
REQUIRE(cubeGltf->parse(fastgltf::Category::OnlyRenderable) == fastgltf::Error::None);
REQUIRE(cubeGltf->validate() == fastgltf::Error::None);

auto cube = cubeGltf->getParsedAsset();
Expand Down Expand Up @@ -154,7 +154,7 @@ TEST_CASE("Loading some basic glTF", "[gltf-loader]") {
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(boxGltf != nullptr);

REQUIRE(boxGltf->parse(fastgltf::Category::Scenes) == fastgltf::Error::None);
REQUIRE(boxGltf->parse(fastgltf::Category::OnlyRenderable) == fastgltf::Error::None);
REQUIRE(boxGltf->validate() == fastgltf::Error::None);

auto box = boxGltf->getParsedAsset();
Expand Down Expand Up @@ -188,7 +188,7 @@ TEST_CASE("Loading KHR_texture_basisu glTF files", "[gltf-loader]") {
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(stainedGlassLamp != nullptr);

REQUIRE(stainedGlassLamp->parse(fastgltf::Category::Textures) == fastgltf::Error::None);
REQUIRE(stainedGlassLamp->parse(fastgltf::Category::Textures | fastgltf::Category::Images) == fastgltf::Error::None);
REQUIRE(stainedGlassLamp->validate() == fastgltf::Error::None);

auto asset = stainedGlassLamp->getParsedAsset();
Expand Down Expand Up @@ -249,7 +249,7 @@ TEST_CASE("Loading glTF animation", "[gltf-loader]") {
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(cube != nullptr);

REQUIRE(cube->parse(fastgltf::Category::Animations) == fastgltf::Error::None);
REQUIRE(cube->parse(fastgltf::Category::OnlyAnimations) == fastgltf::Error::None);
REQUIRE(cube->validate() == fastgltf::Error::None);

auto asset = cube->getParsedAsset();
Expand Down Expand Up @@ -280,7 +280,7 @@ TEST_CASE("Loading glTF skins", "[gltf-loader]") {
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(model != nullptr);

REQUIRE(model->parse(fastgltf::Category::Nodes) == fastgltf::Error::None);
REQUIRE(model->parse(fastgltf::Category::Skins | fastgltf::Category::Nodes) == fastgltf::Error::None);
REQUIRE(model->validate() == fastgltf::Error::None);

auto asset = model->getParsedAsset();
Expand Down Expand Up @@ -432,15 +432,15 @@ TEST_CASE("Test TRS parsing and optional decomposition", "[gltf-loader]") {
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(modelWithMatrix != nullptr);

REQUIRE(modelWithMatrix->parse(fastgltf::Category::Nodes) == fastgltf::Error::None);
REQUIRE(modelWithMatrix->parse(fastgltf::Category::Nodes | fastgltf::Category::Cameras) == fastgltf::Error::None);
REQUIRE(modelWithMatrix->validate() == fastgltf::Error::None);
auto assetWithMatrix = modelWithMatrix->getParsedAsset();

auto modelDecomposed = parser.loadGLTF(jsonData.get(), path, fastgltf::Options::DecomposeNodeMatrices);
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(modelWithMatrix != nullptr);

REQUIRE(modelDecomposed->parse(fastgltf::Category::Nodes) == fastgltf::Error::None);
REQUIRE(modelDecomposed->parse(fastgltf::Category::Nodes | fastgltf::Category::Cameras) == fastgltf::Error::None);
REQUIRE(modelDecomposed->validate() == fastgltf::Error::None);
auto assetDecomposed = modelDecomposed->getParsedAsset();

Expand Down Expand Up @@ -547,7 +547,7 @@ TEST_CASE("Validate morph target parsing", "[gltf-loader]") {
auto model = parser.loadGLTF(jsonData.get(), simpleMorph);
REQUIRE(model != nullptr);
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(model->parse(fastgltf::Category::Nodes) == fastgltf::Error::None);
REQUIRE(model->parse(fastgltf::Category::Meshes) == fastgltf::Error::None);
REQUIRE(model->validate() == fastgltf::Error::None);

auto asset = model->getParsedAsset();
Expand All @@ -574,7 +574,7 @@ TEST_CASE("Test KHR_lights_punctual", "[gltf-loader]") {
fastgltf::Parser parser(fastgltf::Extensions::KHR_lights_punctual);
auto model = parser.loadGLTF(&jsonData, lightsLamp);
REQUIRE(parser.getError() == fastgltf::Error::None);
REQUIRE(model->parse(fastgltf::Category::All) == fastgltf::Error::None);
REQUIRE(model->parse(fastgltf::Category::Nodes) == fastgltf::Error::None);
REQUIRE(model->validate() == fastgltf::Error::None);

auto asset = model->getParsedAsset();
Expand Down

0 comments on commit a334d38

Please sign in to comment.