From 3235a3081c12e99d4bec350eff04b3073c07cdf8 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Mon, 3 Dec 2018 16:19:08 +0100 Subject: Fix T57858: Add validation callback to CustomData layers. Our mesh validation was only checking cd layout so far, not their actual data. While this might only be needed for a few types, this is a required addition for things like imported UVs, else we have no way to avoid nasty things like NANs & co. Note that more layer types may need that callback, time will say. For now added it to some obvious missing cases... --- source/blender/blenkernel/BKE_customdata.h | 4 + source/blender/blenkernel/BKE_mesh.h | 6 +- source/blender/blenkernel/intern/DerivedMesh.c | 8 +- source/blender/blenkernel/intern/customdata.c | 98 +++++++++++++++++++++--- source/blender/blenkernel/intern/mesh_runtime.c | 5 +- source/blender/blenkernel/intern/mesh_validate.c | 33 +++++--- 6 files changed, 127 insertions(+), 27 deletions(-) diff --git a/source/blender/blenkernel/BKE_customdata.h b/source/blender/blenkernel/BKE_customdata.h index 23bc7ddd6fd..0be8e760cae 100644 --- a/source/blender/blenkernel/BKE_customdata.h +++ b/source/blender/blenkernel/BKE_customdata.h @@ -82,6 +82,7 @@ void customData_mask_layers__print(CustomDataMask mask); typedef void (*cd_interp)(const void **sources, const float *weights, const float *sub_weights, int count, void *dest); typedef void (*cd_copy)(const void *source, void *dest, int count); +typedef bool (*cd_validate)(void *item, const uint totitems, const bool do_fixes); /** * Checks if the layer at physical offset \a layer_n (in data->layers) support math @@ -399,6 +400,9 @@ void CustomData_bmesh_init_pool(struct CustomData *data, int totelem, const char bool CustomData_from_bmeshpoly_test(CustomData *fdata, CustomData *ldata, bool fallback); #endif +/* Layer data validation. */ +bool CustomData_layer_validate(struct CustomDataLayer *layer, const uint totitems, const bool do_fixes); + /* External file storage */ void CustomData_external_add(struct CustomData *data, diff --git a/source/blender/blenkernel/BKE_mesh.h b/source/blender/blenkernel/BKE_mesh.h index aba42820519..1e0474fc933 100644 --- a/source/blender/blenkernel/BKE_mesh.h +++ b/source/blender/blenkernel/BKE_mesh.h @@ -487,8 +487,10 @@ bool BKE_mesh_validate_arrays( bool *r_change); bool BKE_mesh_validate_all_customdata( - struct CustomData *vdata, struct CustomData *edata, - struct CustomData *ldata, struct CustomData *pdata, + struct CustomData *vdata, const uint totvert, + struct CustomData *edata, const uint totedge, + struct CustomData *ldata, const uint totloop, + struct CustomData *pdata, const uint totpoly, const bool check_meshmask, const bool do_verbose, const bool do_fixes, bool *r_change); diff --git a/source/blender/blenkernel/intern/DerivedMesh.c b/source/blender/blenkernel/intern/DerivedMesh.c index f0097c46a71..98d2e77c62f 100644 --- a/source/blender/blenkernel/intern/DerivedMesh.c +++ b/source/blender/blenkernel/intern/DerivedMesh.c @@ -2770,10 +2770,10 @@ bool DM_is_valid(DerivedMesh *dm) bool changed = true; is_valid &= BKE_mesh_validate_all_customdata( - dm->getVertDataLayout(dm), - dm->getEdgeDataLayout(dm), - dm->getLoopDataLayout(dm), - dm->getPolyDataLayout(dm), + dm->getVertDataLayout(dm), dm->getNumVerts(dm), + dm->getEdgeDataLayout(dm), dm->getNumEdges(dm), + dm->getLoopDataLayout(dm), dm->getNumLoops(dm), + dm->getPolyDataLayout(dm), dm->getNumPolys(dm), false, /* setting mask here isn't useful, gives false positives */ do_verbose, do_fixes, &changed); diff --git a/source/blender/blenkernel/intern/customdata.c b/source/blender/blenkernel/intern/customdata.c index e841832f51c..b938dbb1d04 100644 --- a/source/blender/blenkernel/intern/customdata.c +++ b/source/blender/blenkernel/intern/customdata.c @@ -126,6 +126,9 @@ typedef struct LayerTypeInfo { * default is assumed to be all zeros */ void (*set_default)(void *data, int count); + /** A function used by mesh validating code, must ensures passed item has valid data. */ + cd_validate validate; + /** functions necessary for geometry collapse */ bool (*equal)(const void *data1, const void *data2); void (*multiply)(void *data, float fac); @@ -313,6 +316,30 @@ static void layerInterp_normal( normalize_v3_v3((float *)dest, no); } +static bool layerValidate_normal(void *data, const uint totitems, const bool do_fixes) +{ + static const float no_default[3] = {0.0f, 0.0f, 1.0f}; /* Z-up default normal... */ + float (*no)[3] = data; + bool has_errors = false; + + for (int i = 0; i < totitems; i++, no++) { + if (!is_finite_v3((float *)no)) { + has_errors = true; + if (do_fixes) { + copy_v3_v3((float *)no, no_default); + } + } + else if (!compare_ff(len_squared_v3((float *)no), 1.0f, 1e-6f)) { + has_errors = true; + if (do_fixes) { + normalize_v3((float *)no); + } + } + } + + return has_errors; +} + static void layerCopyValue_normal(const void *source, void *dest, const int mixmode, const float mixfactor) { const float *no_src = source; @@ -422,6 +449,23 @@ static void layerCopy_propFloat(const void *source, void *dest, memcpy(dest, source, sizeof(MFloatProperty) * count); } +static bool layerValidate_propFloat(void *data, const uint totitems, const bool do_fixes) +{ + MFloatProperty *fp = data; + bool has_errors = false; + + for (int i = 0; i < totitems; i++, fp++) { + if (!isfinite(fp->f)) { + if (do_fixes) { + fp->f = 0.0f; + } + has_errors = true; + } + } + + return has_errors; +} + static void layerCopy_propInt(const void *source, void *dest, int count) { @@ -908,6 +952,23 @@ static void layerInterp_mloopuv( ((MLoopUV *)dest)->flag = flag; } +static bool layerValidate_mloopuv(void *data, const uint totitems, const bool do_fixes) +{ + MLoopUV *uv = data; + bool has_errors = false; + + for (int i = 0; i < totitems; i++, uv++) { + if (!is_finite_v2(uv->uv)) { + if (do_fixes) { + zero_v2(uv->uv); + } + has_errors = true; + } + } + + return has_errors; +} + /* origspace is almost exact copy of mloopuv's, keep in sync */ static void layerCopyValue_mloop_origspace(const void *source, void *dest, const int UNUSED(mixmode), const float UNUSED(mixfactor)) @@ -1192,21 +1253,22 @@ static const LayerTypeInfo LAYERTYPEINFO[CD_NUMTYPES] = { {sizeof(MFace), "MFace", 1, NULL, NULL, NULL, NULL, NULL, NULL}, /* 5: CD_MTFACE */ {sizeof(MTFace), "MTFace", 1, N_("UVMap"), layerCopy_tface, NULL, layerInterp_tface, layerSwap_tface, - layerDefault_tface, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, layerMaxNum_tface}, + layerDefault_tface, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, layerMaxNum_tface}, /* 6: CD_MCOL */ /* 4 MCol structs per face */ - {sizeof(MCol) * 4, "MCol", 4, N_("Col"), NULL, NULL, layerInterp_mcol, - layerSwap_mcol, layerDefault_mcol, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, layerMaxNum_mloopcol}, + {sizeof(MCol) * 4, "MCol", 4, N_("Col"), NULL, NULL, layerInterp_mcol, layerSwap_mcol, + layerDefault_mcol, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, layerMaxNum_mloopcol}, /* 7: CD_ORIGINDEX */ {sizeof(int), "", 0, NULL, NULL, NULL, NULL, NULL, layerDefault_origindex}, /* 8: CD_NORMAL */ /* 3 floats per normal vector */ {sizeof(float) * 3, "vec3f", 1, NULL, NULL, NULL, layerInterp_normal, NULL, NULL, - NULL, NULL, NULL, NULL, NULL, layerCopyValue_normal}, + layerValidate_normal, NULL, NULL, NULL, NULL, NULL, layerCopyValue_normal}, /* 9: CD_FACEMAP */ {sizeof(int), "", 0, NULL, NULL, NULL, NULL, NULL, layerDefault_fmap, NULL}, /* 10: CD_PROP_FLT */ - {sizeof(MFloatProperty), "MFloatProperty", 1, N_("Float"), layerCopy_propFloat, NULL, NULL, NULL}, + {sizeof(MFloatProperty), "MFloatProperty", 1, N_("Float"), layerCopy_propFloat, NULL, NULL, NULL, NULL, + layerValidate_propFloat}, /* 11: CD_PROP_INT */ {sizeof(MIntProperty), "MIntProperty", 1, N_("Int"), layerCopy_propInt, NULL, NULL, NULL}, /* 12: CD_PROP_STR */ @@ -1221,18 +1283,18 @@ static const LayerTypeInfo LAYERTYPEINFO[CD_NUMTYPES] = { {sizeof(int), "", 0, NULL, NULL, NULL, NULL, NULL, NULL}, /* 16: CD_MLOOPUV */ {sizeof(MLoopUV), "MLoopUV", 1, N_("UVMap"), NULL, NULL, layerInterp_mloopuv, NULL, NULL, - layerEqual_mloopuv, layerMultiply_mloopuv, layerInitMinMax_mloopuv, + layerValidate_mloopuv, layerEqual_mloopuv, layerMultiply_mloopuv, layerInitMinMax_mloopuv, layerAdd_mloopuv, layerDoMinMax_mloopuv, layerCopyValue_mloopuv, NULL, NULL, NULL, layerMaxNum_tface}, /* 17: CD_MLOOPCOL */ {sizeof(MLoopCol), "MLoopCol", 1, N_("Col"), NULL, NULL, layerInterp_mloopcol, NULL, - layerDefault_mloopcol, layerEqual_mloopcol, layerMultiply_mloopcol, layerInitMinMax_mloopcol, + layerDefault_mloopcol, NULL, layerEqual_mloopcol, layerMultiply_mloopcol, layerInitMinMax_mloopcol, layerAdd_mloopcol, layerDoMinMax_mloopcol, layerCopyValue_mloopcol, NULL, NULL, NULL, layerMaxNum_mloopcol}, /* 18: CD_TANGENT */ {sizeof(float) * 4 * 4, "", 0, N_("Tangent"), NULL, NULL, NULL, NULL, NULL}, /* 19: CD_MDISPS */ {sizeof(MDisps), "MDisps", 1, NULL, layerCopy_mdisps, layerFree_mdisps, NULL, layerSwap_mdisps, NULL, - NULL, NULL, NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, NULL, NULL, NULL, layerRead_mdisps, layerWrite_mdisps, layerFilesize_mdisps}, /* 20: CD_PREVIEW_MCOL */ {sizeof(MCol) * 4, "MCol", 4, N_("PreviewCol"), NULL, NULL, layerInterp_mcol, @@ -1261,12 +1323,12 @@ static const LayerTypeInfo LAYERTYPEINFO[CD_NUMTYPES] = { /* 30: CD_CREASE */ {sizeof(float), "", 0, N_("SubSurfCrease"), NULL, NULL, layerInterp_bweight}, /* 31: CD_ORIGSPACE_MLOOP */ - {sizeof(OrigSpaceLoop), "OrigSpaceLoop", 1, N_("OS Loop"), NULL, NULL, layerInterp_mloop_origspace, NULL, NULL, + {sizeof(OrigSpaceLoop), "OrigSpaceLoop", 1, N_("OS Loop"), NULL, NULL, layerInterp_mloop_origspace, NULL, NULL, NULL, layerEqual_mloop_origspace, layerMultiply_mloop_origspace, layerInitMinMax_mloop_origspace, layerAdd_mloop_origspace, layerDoMinMax_mloop_origspace, layerCopyValue_mloop_origspace}, /* 32: CD_PREVIEW_MLOOPCOL */ {sizeof(MLoopCol), "MLoopCol", 1, N_("PreviewLoopCol"), NULL, NULL, layerInterp_mloopcol, NULL, - layerDefault_mloopcol, layerEqual_mloopcol, layerMultiply_mloopcol, layerInitMinMax_mloopcol, + layerDefault_mloopcol, NULL, layerEqual_mloopcol, layerMultiply_mloopcol, layerInitMinMax_mloopcol, layerAdd_mloopcol, layerDoMinMax_mloopcol, layerCopyValue_mloopcol}, /* 33: CD_BM_ELEM_PYPTR */ {sizeof(void *), "", 1, NULL, layerCopy_bmesh_elem_py_ptr, @@ -3509,6 +3571,22 @@ bool CustomData_verify_versions(struct CustomData *data, int index) return keeplayer; } +/** + * Validate and fix data of \a layer, if possible (needs relevant callback in layer's type to be defined). + * + * \return True if some errors were found. + */ +bool CustomData_layer_validate(CustomDataLayer *layer, const uint totitems, const bool do_fixes) +{ + const LayerTypeInfo *typeInfo = layerType_getInfo(layer->type); + + if (typeInfo->validate != NULL) { + return typeInfo->validate(layer->data, totitems, do_fixes); + } + + return false; +} + /****************************** External Files *******************************/ static void customdata_external_filename(char filename[FILE_MAX], ID *id, CustomDataExternal *external) diff --git a/source/blender/blenkernel/intern/mesh_runtime.c b/source/blender/blenkernel/intern/mesh_runtime.c index 0931318f17a..14d77b5288b 100644 --- a/source/blender/blenkernel/intern/mesh_runtime.c +++ b/source/blender/blenkernel/intern/mesh_runtime.c @@ -354,7 +354,10 @@ bool BKE_mesh_runtime_is_valid(Mesh *me_eval) } is_valid &= BKE_mesh_validate_all_customdata( - &me_eval->vdata, &me_eval->edata, &me_eval->ldata, &me_eval->pdata, + &me_eval->vdata, me_eval->totvert, + &me_eval->edata, me_eval->totedge, + &me_eval->ldata, me_eval->totloop, + &me_eval->pdata, me_eval->totpoly, false, /* setting mask here isn't useful, gives false positives */ do_verbose, do_fixes, &changed); diff --git a/source/blender/blenkernel/intern/mesh_validate.c b/source/blender/blenkernel/intern/mesh_validate.c index af4d1265cfd..bb86e4bc673 100644 --- a/source/blender/blenkernel/intern/mesh_validate.c +++ b/source/blender/blenkernel/intern/mesh_validate.c @@ -820,7 +820,7 @@ bool BKE_mesh_validate_arrays( } static bool mesh_validate_customdata( - CustomData *data, CustomDataMask mask, + CustomData *data, CustomDataMask mask, const uint totitems, const bool do_verbose, const bool do_fixes, bool *r_change) { @@ -859,8 +859,13 @@ static bool mesh_validate_customdata( } } - if (ok) + if (ok) { + if (CustomData_layer_validate(layer, totitems, do_fixes)) { + PRINT_ERR("\tCustomDataLayer type %d has some invalid data\n", layer->type); + has_fixes = do_fixes; + } i++; + } } PRINT_MSG("%s: Finished (is_valid=%d)\n\n", __func__, (int)!has_fixes); @@ -874,8 +879,10 @@ static bool mesh_validate_customdata( * \returns is_valid. */ bool BKE_mesh_validate_all_customdata( - CustomData *vdata, CustomData *edata, - CustomData *ldata, CustomData *pdata, + CustomData *vdata, const uint totvert, + CustomData *edata, const uint totedge, + CustomData *ldata, const uint totloop, + CustomData *pdata, const uint totpoly, const bool check_meshmask, const bool do_verbose, const bool do_fixes, bool *r_change) @@ -885,10 +892,10 @@ bool BKE_mesh_validate_all_customdata( int tot_uvloop, tot_vcolloop; CustomDataMask mask = check_meshmask ? CD_MASK_MESH : 0; - is_valid &= mesh_validate_customdata(vdata, mask, do_verbose, do_fixes, &is_change_v); - is_valid &= mesh_validate_customdata(edata, mask, do_verbose, do_fixes, &is_change_e); - is_valid &= mesh_validate_customdata(ldata, mask, do_verbose, do_fixes, &is_change_l); - is_valid &= mesh_validate_customdata(pdata, mask, do_verbose, do_fixes, &is_change_p); + is_valid &= mesh_validate_customdata(vdata, mask, totvert, do_verbose, do_fixes, &is_change_v); + is_valid &= mesh_validate_customdata(edata, mask, totedge, do_verbose, do_fixes, &is_change_e); + is_valid &= mesh_validate_customdata(ldata, mask, totloop, do_verbose, do_fixes, &is_change_l); + is_valid &= mesh_validate_customdata(pdata, mask, totpoly, do_verbose, do_fixes, &is_change_p); tot_uvloop = CustomData_number_of_layers(ldata, CD_MLOOPUV); tot_vcolloop = CustomData_number_of_layers(ldata, CD_MLOOPCOL); @@ -931,7 +938,10 @@ bool BKE_mesh_validate(Mesh *me, const bool do_verbose, const bool cddata_check_ } is_valid &= BKE_mesh_validate_all_customdata( - &me->vdata, &me->edata, &me->ldata, &me->pdata, + &me->vdata, me->totvert, + &me->edata, me->totedge, + &me->ldata, me->totloop, + &me->pdata, me->totpoly, cddata_check_mask, do_verbose, true, &changed); @@ -972,7 +982,10 @@ bool BKE_mesh_is_valid(Mesh *me) bool changed = true; is_valid &= BKE_mesh_validate_all_customdata( - &me->vdata, &me->edata, &me->ldata, &me->pdata, + &me->vdata, me->totvert, + &me->edata, me->totedge, + &me->ldata, me->totloop, + &me->pdata, me->totpoly, false, /* setting mask here isn't useful, gives false positives */ do_verbose, do_fixes, &changed); -- cgit v1.2.3