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

Some update to gltf loading #4373

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Some update to gltf loading #4373

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 27 additions & 19 deletions src/rmodels.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5226,17 +5226,19 @@ static Model LoadGLTF(const char *fileName)
***********************************************************************************************/

// Macro to simplify attributes loading code
#define LOAD_ATTRIBUTE(accesor, numComp, dataType, dstPtr) \
#define LOAD_ATTRIBUTE(accesor, numComp, srcType, dstPtr) LOAD_ATTRIBUTE_CAST(accesor, numComp, srcType, dstPtr, srcType)

#define LOAD_ATTRIBUTE_CAST(accesor, numComp, srcType, dstPtr, dstType) \
Copy link
Contributor Author

@HarryDC HarryDC Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@raysan5 mention in discord this would have to change to include something like

if (dstPtr == NULL) { 
  dstPtr = RL_ALLOC(accessor->count * numComp * sizeof(dstType); 
}

This would reduce a lot of the 4-6 line if/else branches to 1 Line

LOAD_ATTRIBUTE(accessor, numpComp, srcType, dstPtr, dstType)

As this touches every if branch in the gltf load i didn't even want to start that before checking

Copy link
Owner

Choose a reason for hiding this comment

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

Personally, I don't like that macro at all, it could be confusing and it's difficult/impossible to debug.

Said that, proposed improvement sounds good to me. Feel free to send a PR with the required updates.

{ \
int n = 0; \
dataType *buffer = (dataType *)accesor->buffer_view->buffer->data + accesor->buffer_view->offset/sizeof(dataType) + accesor->offset/sizeof(dataType); \
srcType *buffer = (srcType *)accesor->buffer_view->buffer->data + accesor->buffer_view->offset/sizeof(srcType) + accesor->offset/sizeof(srcType); \
for (unsigned int k = 0; k < accesor->count; k++) \
{\
for (int l = 0; l < numComp; l++) \
{\
dstPtr[numComp*k + l] = buffer[n + l];\
dstPtr[numComp*k + l] = (dstType)buffer[n + l];\
}\
n += (int)(accesor->stride/sizeof(dataType));\
n += (int)(accesor->stride/sizeof(srcType));\
}\
}

Expand Down Expand Up @@ -5697,23 +5699,25 @@ static Model LoadGLTF(const char *fileName)
// Load unsigned short data type into mesh.indices
LOAD_ATTRIBUTE(attribute, 1, unsigned short, model.meshes[meshIndex].indices)
}
else if (attribute->component_type == cgltf_component_type_r_8u)
{
// Init raylib mesh indices to copy glTF attribute data
Copy link
Contributor Author

@HarryDC HarryDC Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With allocation inside the macro, this would be one line

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good to me.

model.meshes[meshIndex].indices = RL_MALLOC(attribute->count * sizeof(unsigned short));
LOAD_ATTRIBUTE_CAST(attribute, 1, unsigned char, model.meshes[meshIndex].indices, unsigned short)

}
else if (attribute->component_type == cgltf_component_type_r_32u)
{
// Init raylib mesh indices to copy glTF attribute data
model.meshes[meshIndex].indices = RL_MALLOC(attribute->count*sizeof(unsigned short));

// Load data into a temp buffer to be converted to raylib data type
unsigned int *temp = RL_MALLOC(attribute->count*sizeof(unsigned int));
LOAD_ATTRIBUTE(attribute, 1, unsigned int, temp);

// Convert data to raylib indices data type (unsigned short)
for (unsigned int d = 0; d < attribute->count; d++) model.meshes[meshIndex].indices[d] = (unsigned short)temp[d];
LOAD_ATTRIBUTE_CAST(attribute, 1, unsigned int, model.meshes[meshIndex].indices, unsigned short);

TRACELOG(LOG_WARNING, "MODEL: [%s] Indices data converted from u32 to u16, possible loss of data", fileName);

RL_FREE(temp);
}
else TRACELOG(LOG_WARNING, "MODEL: [%s] Indices data format not supported, use u16", fileName);
else
{
TRACELOG(LOG_WARNING, "MODEL: [%s] Indices data format not supported, use u16", fileName);
}
}
else model.meshes[meshIndex].triangleCount = model.meshes[meshIndex].vertexCount/3; // Unindexed mesh

Expand Down Expand Up @@ -5745,7 +5749,7 @@ static Model LoadGLTF(const char *fileName)
// - Only supports linear interpolation (default method in Blender when checked "Always Sample Animations" when exporting a GLTF file)
// - Only supports translation/rotation/scale animation channel.path, weights not considered (i.e. morph targets)
//----------------------------------------------------------------------------------------------------
if (data->skins_count == 1)
if (data->skins_count > 0)
{
cgltf_skin skin = data->skins[0];
model.bones = LoadBoneInfoGLTF(skin, &model.boneCount);
Expand All @@ -5765,9 +5769,9 @@ static Model LoadGLTF(const char *fileName)
MatrixDecompose(worldMatrix, &(model.bindPose[i].translation), &(model.bindPose[i].rotation), &(model.bindPose[i].scale));
}
}
else if (data->skins_count > 1)
if (data->skins_count > 1)
{
TRACELOG(LOG_ERROR, "MODEL: [%s] can only load one skin (armature) per model, but gltf skins_count == %i", fileName, data->skins_count);
TRACELOG(LOG_WARNING, "MODEL: [%s] can only load one skin (armature) per model, but gltf skins_count == %i", fileName, data->skins_count);
}

meshIndex = 0;
Expand Down Expand Up @@ -6088,7 +6092,7 @@ static ModelAnimation *LoadModelAnimationsGLTF(const char *fileName, int *animCo

if (result == cgltf_result_success)
{
if (data->skins_count == 1)
if (data->skins_count > 0)
{
cgltf_skin skin = data->skins[0];
*animCount = (int)data->animations_count;
Expand Down Expand Up @@ -6223,7 +6227,11 @@ static ModelAnimation *LoadModelAnimationsGLTF(const char *fileName, int *animCo
RL_FREE(boneChannels);
}
}
else TRACELOG(LOG_ERROR, "MODEL: [%s] expected exactly one skin to load animation data from, but found %i", fileName, data->skins_count);

if (data->skins_count > 1)
{
TRACELOG(LOG_WARNING, "MODEL: [%s] expected exactly one skin to load animation data from, but found %i", fileName, data->skins_count);
}

cgltf_free(data);
}
Expand Down