From f758c4a6b1da14422b9b5d5632b3fdb3e5311363 Mon Sep 17 00:00:00 2001 From: Sietse Brouwer Date: Mon, 19 Jun 2023 15:49:57 +0200 Subject: [PATCH 1/2] Fix #108935: Auto-Normalize glitches in GPencil weight painting There where glitches while weight painting in Grease Pencil with auto-normalize enabled. This resulted in incorrect weights on vertices. And sometimes a vertex group not deformed by bones was also affected by auto-normalize (which should never happen). This was due to: - An erroneous line in the auto-normalize function which led to an index out of bounds (and screwing up the vertex weights). - An edge case where the active vertex group was de facto the only group to be normalized. Also in this patch: - Removal of unreachable code - Better names for the auto-normalize functions Pull Request: https://projects.blender.org/blender/blender/pulls/109036 --- .../gpencil_legacy/gpencil_weight_paint.c | 95 +++++-------------- 1 file changed, 22 insertions(+), 73 deletions(-) diff --git a/source/blender/editors/gpencil_legacy/gpencil_weight_paint.c b/source/blender/editors/gpencil_legacy/gpencil_weight_paint.c index 212914ad662..080e258432e 100644 --- a/source/blender/editors/gpencil_legacy/gpencil_weight_paint.c +++ b/source/blender/editors/gpencil_legacy/gpencil_weight_paint.c @@ -346,72 +346,19 @@ static bool *gpencil_vgroup_bone_deformed_map_get(Object *ob, const int defbase_ * for points in vertex groups deformed by bones. * The logic is copied from `editors/sculpt_paint/paint_vertex.cc`. */ -/** - * Normalize weights of a stroke point deformed by bones. - */ -static void do_weight_paint_normalize_all(MDeformVert *dvert, - const int defbase_tot, - const bool *vgroup_bone_deformed) -{ - float sum = 0.0f, fac; - uint tot = 0; - MDeformWeight *dw; - - dw = dvert->dw; - for (int i = dvert->totweight; i != 0; i--, dw++) { - if (dw->def_nr < defbase_tot && vgroup_bone_deformed[dw->def_nr]) { - tot++; - sum += dw->weight; - } - } - - if ((tot == 0) || (sum == 1.0f)) { - return; - } - - if (sum != 0.0f) { - fac = 1.0f / sum; - - dw = dvert->dw; - for (int i = dvert->totweight; i != 0; i--, dw++) { - if (dw->def_nr < defbase_tot && vgroup_bone_deformed[dw->def_nr]) { - dw->weight *= fac; - } - } - } - else { - fac = 1.0f / tot; - - dw = dvert->dw; - for (int i = dvert->totweight; i != 0; i--, dw++) { - if (dw->def_nr < defbase_tot && vgroup_bone_deformed[dw->def_nr]) { - dw->weight = fac; - } - } - } -} - -/** - * A version of #do_weight_paint_normalize_all that only changes unlocked weights. - */ -static bool do_weight_paint_normalize_all_unlocked(MDeformVert *dvert, - const int defbase_tot, - const bool *vgroup_bone_deformed, - const bool *vgroup_locked, - const bool lock_active_vgroup, - const int active_vgroup) +static bool do_weight_paint_normalize(MDeformVert *dvert, + const int defbase_tot, + const bool *vgroup_bone_deformed, + const bool *vgroup_locked, + const bool lock_active_vgroup, + const int active_vgroup) { float sum = 0.0f, fac; float sum_unlock = 0.0f; float sum_lock = 0.0f; - uint tot = 0; + uint lock_tot = 0, unlock_tot = 0; MDeformWeight *dw; - if (vgroup_locked == NULL) { - do_weight_paint_normalize_all(dvert, defbase_tot, vgroup_bone_deformed); - return true; - } - if (dvert->totweight <= 1) { return true; } @@ -423,10 +370,11 @@ static bool do_weight_paint_normalize_all_unlocked(MDeformVert *dvert, sum += dw->weight; if (vgroup_locked[dw->def_nr] || (lock_active_vgroup && active_vgroup == dw->def_nr)) { + lock_tot++; sum_lock += dw->weight; } else { - tot++; + unlock_tot++; sum_unlock += dw->weight; } } @@ -436,8 +384,10 @@ static bool do_weight_paint_normalize_all_unlocked(MDeformVert *dvert, return true; } - if (tot == 0) { - return false; + if (unlock_tot == 0) { + /* There are no unlocked vertex groups to normalize. We don't need + * a second pass when there is only one locked group (the active group). */ + return (lock_tot == 1); } if (sum_lock >= 1.0f - VERTEX_WEIGHT_LOCK_EPSILON) { @@ -445,7 +395,6 @@ static bool do_weight_paint_normalize_all_unlocked(MDeformVert *dvert, * zero out what we can and return false. */ dw = dvert->dw; for (int i = dvert->totweight; i != 0; i--, dw++) { - *dw = dvert->dw[i]; if (dw->def_nr < defbase_tot && vgroup_bone_deformed[dw->def_nr]) { if ((vgroup_locked[dw->def_nr] == false) && !(lock_active_vgroup && active_vgroup == dw->def_nr)) { @@ -472,7 +421,7 @@ static bool do_weight_paint_normalize_all_unlocked(MDeformVert *dvert, } } else { - fac = (1.0f - sum_lock) / tot; + fac = (1.0f - sum_lock) / unlock_tot; CLAMP(fac, 0.0f, 1.0f); dw = dvert->dw; @@ -491,18 +440,18 @@ static bool do_weight_paint_normalize_all_unlocked(MDeformVert *dvert, } /** - * A version of #do_weight_paint_normalize_all that only changes unlocked weights + * A version of #do_weight_paint_normalize that only changes unlocked weights * and does a second pass without the active vertex group locked when the first pass fails. */ -static void do_weight_paint_normalize_all_try(MDeformVert *dvert, tGP_BrushWeightpaintData *gso) +static void do_weight_paint_normalize_try(MDeformVert *dvert, tGP_BrushWeightpaintData *gso) { /* First pass with both active and explicitly locked vertex groups restricted from change. */ - bool succes = do_weight_paint_normalize_all_unlocked( + bool succes = do_weight_paint_normalize( dvert, gso->vgroup_tot, gso->vgroup_bone_deformed, gso->vgroup_locked, true, gso->vrgroup); if (!succes) { /* Second pass with active vertex group unlocked. */ - do_weight_paint_normalize_all_unlocked( + do_weight_paint_normalize( dvert, gso->vgroup_tot, gso->vgroup_bone_deformed, gso->vgroup_locked, false, -1); } } @@ -584,7 +533,7 @@ static bool brush_draw_apply(tGP_BrushWeightpaintData *gso, /* Perform auto-normalize. */ if (gso->auto_normalize) { - do_weight_paint_normalize_all_try(dvert, gso); + do_weight_paint_normalize_try(dvert, gso); } return true; @@ -614,7 +563,7 @@ static bool brush_average_apply(tGP_BrushWeightpaintData *gso, /* Perform auto-normalize. */ if (gso->auto_normalize) { - do_weight_paint_normalize_all_try(dvert, gso); + do_weight_paint_normalize_try(dvert, gso); } return true; @@ -666,7 +615,7 @@ static bool brush_blur_apply(tGP_BrushWeightpaintData *gso, /* Perform auto-normalize. */ if (gso->auto_normalize) { - do_weight_paint_normalize_all_try(dvert, gso); + do_weight_paint_normalize_try(dvert, gso); } return true; @@ -748,7 +697,7 @@ static bool brush_smear_apply(tGP_BrushWeightpaintData *gso, /* Perform auto-normalize. */ if (gso->auto_normalize) { - do_weight_paint_normalize_all_try(dvert, gso); + do_weight_paint_normalize_try(dvert, gso); } return true; From 232e065a17ba007bfe346c201494eabed14baa8f Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Mon, 19 Jun 2023 16:21:09 +0200 Subject: [PATCH 2/2] Fix #109053: File Append with recursions leads to crash In this specific code path (recursive reading inside .blend files containing assets), reading datablocks marked as asset would move ownership over the asset metadata without indicating that in the source that owned it previously. This would cause a double free attempt. --- source/blender/blenloader/BLO_readfile.h | 2 ++ source/blender/editors/include/ED_file_indexer.h | 5 ++++- source/blender/editors/space_file/file_indexer.cc | 12 +++++++----- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/source/blender/blenloader/BLO_readfile.h b/source/blender/blenloader/BLO_readfile.h index 58896d432a8..9d351cd66bb 100644 --- a/source/blender/blenloader/BLO_readfile.h +++ b/source/blender/blenloader/BLO_readfile.h @@ -181,6 +181,8 @@ void BLO_blendfiledata_free(BlendFileData *bfd); typedef struct BLODataBlockInfo { char name[64]; /* MAX_NAME */ struct AssetMetaData *asset_data; + /** Ownership over #asset_data above can be "stolen out" of this struct, for more permanent + * storage. In that case, set this to false to avoid double freeing of the stolen data. */ bool free_asset_data; /* Optimization: Tag data-blocks for which we know there is no preview. * Knowing this can be used to skip the (potentially expensive) preview loading process. If this diff --git a/source/blender/editors/include/ED_file_indexer.h b/source/blender/editors/include/ED_file_indexer.h index 981a6015a94..a1a0b73c2a3 100644 --- a/source/blender/editors/include/ED_file_indexer.h +++ b/source/blender/editors/include/ED_file_indexer.h @@ -127,10 +127,13 @@ void ED_file_indexer_entries_clear(FileIndexerEntries *indexer_entries); * Adds all entries from the given `datablock_infos` to the `indexer_entries`. * The datablock_infos must only contain data for a single IDType. The specific IDType must be * passed in the `idcode` parameter. + * + * \note This can "steal" data contained in \a datablock_infos, to avoid expensive copies, which is + * supported by the #BLODataBlockInfo type. */ void ED_file_indexer_entries_extend_from_datablock_infos( FileIndexerEntries *indexer_entries, - const LinkNode * /*BLODataBlockInfo*/ datablock_infos, + struct LinkNode * /*BLODataBlockInfo*/ datablock_infos, int idcode); #ifdef __cplusplus diff --git a/source/blender/editors/space_file/file_indexer.cc b/source/blender/editors/space_file/file_indexer.cc index 4ee925c256d..b1c65cf2798 100644 --- a/source/blender/editors/space_file/file_indexer.cc +++ b/source/blender/editors/space_file/file_indexer.cc @@ -40,12 +40,14 @@ constexpr FileIndexerType default_indexer() } static FileIndexerEntry *file_indexer_entry_create_from_datablock_info( - const BLODataBlockInfo *datablock_info, const int idcode) + BLODataBlockInfo *datablock_info, const int idcode) { FileIndexerEntry *entry = static_cast( MEM_mallocN(sizeof(FileIndexerEntry), __func__)); - entry->datablock_info = *datablock_info; entry->idcode = idcode; + /* Shallow copy data-block info and mark original as having its asset data ownership stolen. */ + entry->datablock_info = *datablock_info; + datablock_info->free_asset_data = false; return entry; } @@ -55,11 +57,11 @@ extern "C" { void ED_file_indexer_entries_extend_from_datablock_infos( FileIndexerEntries *indexer_entries, - const LinkNode * /*BLODataBlockInfo*/ datablock_infos, + LinkNode * /*BLODataBlockInfo*/ datablock_infos, const int idcode) { - for (const LinkNode *ln = datablock_infos; ln; ln = ln->next) { - const BLODataBlockInfo *datablock_info = static_cast(ln->link); + for (LinkNode *ln = datablock_infos; ln; ln = ln->next) { + BLODataBlockInfo *datablock_info = static_cast(ln->link); FileIndexerEntry *file_indexer_entry = blender::ed::file::indexer::file_indexer_entry_create_from_datablock_info(datablock_info, idcode);