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

Utilities to extract data from accessors #20

Merged
merged 26 commits into from
May 16, 2023
Merged

Conversation

forenoonwatch
Copy link
Contributor

@forenoonwatch forenoonwatch commented Apr 26, 2023

This PR adds fastgltf_tools.hpp, a header containing several utility functions such as getAccessorElement, iterateAccessor, and fillTargetFromAccessor, the ElementTraits<T> struct for matching metadata about the desired type, and the DefaultBufferDataAdapter, a minimal default implementation of the BufferDataAdapter pattern which allows the accessor indexing functions to resolve used buffer memory.

Currently this PR lacks support for compressed buffer views from EXT_meshopt_compression.

@forenoonwatch forenoonwatch marked this pull request as draft April 26, 2023 18:53
@forenoonwatch forenoonwatch marked this pull request as ready for review April 26, 2023 18:53
Copy link
Owner

@spnda spnda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few general things:

  • Please adapt the else style to the rest of the repo, as well as the naming (currently snake_case) of the template specialisation constants.
  • Probably should add some static_asserts that the required ElementTraits all exist, and that certain template types meet some criteria (could use concepts with a C++20 preprocessor guard).

src/fastgltf_types.hpp Outdated Show resolved Hide resolved
tests/accessor_tests.cpp Outdated Show resolved Hide resolved
tests/accessor_tests.cpp Outdated Show resolved Hide resolved
tests/accessor_tests.cpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
Copy link
Owner

@spnda spnda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I'd like to see more comments around the top level functions and in some complex situations for the future.
  • In Release mode on MSVC the "getAccessorElement" test fails sometimes (!) with sparse accessors. With a debugger I noticed the values are definitely wrong but I couldn't tell you why.

REQUIRE(checkValues[i] == fastgltf::getAccessorElementglm::vec3(*asset, secondAccessor, i))
with expansion:
{?} == {?}

src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
tests/accessor_tests.cpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
@forenoonwatch
Copy link
Contributor Author

  • In Release mode on MSVC the "getAccessorElement" test fails sometimes (!) with sparse accessors. With a debugger I noticed the values are definitely wrong but I couldn't tell you why.

REQUIRE(checkValues[i] == fastgltf::getAccessorElementglm::vec3(*asset, secondAccessor, i))
with expansion:
{?} == {?}

I've been able to replicate this in my MinGW release (intermittently). Trying to see if I can actually figure out what's causing it.

tests/accessor_tests.cpp Outdated Show resolved Hide resolved
tests/accessor_tests.cpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
tests/accessor_tests.cpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
src/fastgltf_tools.hpp Outdated Show resolved Hide resolved
@spnda spnda merged commit 3815dc8 into spnda:main May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants