From 39c1127dc06b5fa1c8ab07fe3f78d96f87e16fd9 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Mon, 14 Oct 2024 13:59:06 +0200 Subject: [PATCH] Fixes for #128078 # Fix 128078, Part I: Fix missing reverse endian switch of Action's `idroot`. ID type code stored as ints (or shorts) need their endian switch to be reverted (in case there is endianess conversion) on file read. Interestingly, this was done for the deprecated IPO data (among others), but not for the Action one! NOTE: There is no versioning fix for this mistake, i.e. old files that were saved from a BE system, then opened and re-saved from a LE system, will still have totally invalid ID code values. This is not considered as necessary currently, given that this `idroot` value is only 'informational' and not relied on by any part of the code. # Fix 128078, Part II: GPv3 conversion code missing animation of Layers' location. Also add code to the AnimDataConverter to ensure that actions get the `idroot` matching their new ID owner type in GP data case. Pull Request: https://projects.blender.org/blender/blender/pulls/128129 --- source/blender/blenkernel/intern/action.cc | 10 +++ .../intern/grease_pencil_convert_legacy.cc | 85 +++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/source/blender/blenkernel/intern/action.cc b/source/blender/blenkernel/intern/action.cc index 123f6a183b6..9666d275d28 100644 --- a/source/blender/blenkernel/intern/action.cc +++ b/source/blender/blenkernel/intern/action.cc @@ -24,6 +24,7 @@ #include "DNA_scene_types.h" #include "BLI_blenlib.h" +#include "BLI_endian_switch.h" #include "BLI_ghash.h" #include "BLI_math_color.h" #include "BLI_math_matrix.h" @@ -35,6 +36,8 @@ #include "BLT_translation.hh" +#include "BLO_read_write.hh" + #include "BKE_action.hh" #include "BKE_anim_data.hh" #include "BKE_anim_visualization.h" @@ -675,6 +678,13 @@ static void action_blend_read_data(BlendDataReader *reader, ID *id) { animrig::Action &action = reinterpret_cast(id)->wrap(); + /* Undo generic endian switching (careful, only the two least significant bytes of the int32 must + * be swapped back here, since this value is actually an int16). */ + if (BLO_read_requires_endian_switch(reader)) { + bAction *act = reinterpret_cast(id); + BLI_endian_switch_int16(reinterpret_cast(&act->idroot)); + } + #ifdef WITH_ANIM_BAKLAVA read_strip_keyframe_data_array(reader, action); read_layers(reader, action); diff --git a/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc b/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc index d5baf5dbca8..7df50087b88 100644 --- a/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc +++ b/source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc @@ -217,6 +217,7 @@ class AnimDataConvertor { private: using FCurveCallback = bool(bAction *owner_action, FCurve &fcurve); + using ActionCallback = bool(bAction &action); /** \return True if this AnimDataConvertor is valid, i.e. can be used to process animation data * from source ID. */ @@ -324,6 +325,9 @@ class AnimDataConvertor { is_changed = is_changed || local_is_changed; } + /* NOTE: New layered actions system can be ignored here, it did not exist together with GPv2. + */ + if (this->skip_nla) { return is_changed; } @@ -337,6 +341,55 @@ class AnimDataConvertor { return is_changed; } + bool action_process(bAction &action, blender::FunctionRef callback) const + { + if (callback(action)) { + DEG_id_tag_update(&action.id, ID_RECALC_ANIMATION); + return true; + } + return false; + } + + bool nla_strip_action_foreach(NlaStrip &nla_strip, + blender::FunctionRef callback) const + { + bool is_changed = false; + if (nla_strip.act) { + is_changed = action_process(*nla_strip.act, callback); + } + LISTBASE_FOREACH (NlaStrip *, nla_strip_children, &nla_strip.strips) { + is_changed = is_changed || this->nla_strip_action_foreach(*nla_strip_children, callback); + } + return is_changed; + } + + bool animdata_action_foreach(AnimData &anim_data, + blender::FunctionRef callback) const + { + bool is_changed = false; + + if (anim_data.action) { + is_changed = is_changed || action_process(*anim_data.action, callback); + } + if (anim_data.tmpact) { + is_changed = is_changed || action_process(*anim_data.tmpact, callback); + } + + /* NOTE: New layered actions system can be ignored here, it did not exist together with GPv2. + */ + + if (this->skip_nla) { + return is_changed; + } + + LISTBASE_FOREACH (NlaTrack *, nla_track, &anim_data.nla_tracks) { + LISTBASE_FOREACH (NlaStrip *, nla_strip, &nla_track->strips) { + is_changed = is_changed || this->nla_strip_action_foreach(*nla_strip, callback); + } + } + return is_changed; + } + public: /** * Check whether the source animation data contains FCurves that need to be converted/moved to @@ -348,6 +401,10 @@ class AnimDataConvertor { return false; } + if (GS(id_src.name) != GS(id_dst.name)) { + return true; + } + bool has_animation = false; auto animation_detection_cb = [&](bAction *owner_action, FCurve &fcurve) -> bool { /* Early out if we already know that the target data is animated. */ @@ -472,6 +529,19 @@ class AnimDataConvertor { return; } + /* Ensure existing actions moved to a different ID type keep a 'valid' `idroot` value. Not + * essential, but 'nice to have'. */ + if (GS(this->id_src.name) != GS(this->id_dst.name)) { + if (!this->animdata_dst) { + this->animdata_dst = BKE_animdata_ensure_id(&this->id_dst); + } + auto actions_idroot_ensure = [&](bAction &action) -> bool { + action.idroot = GS(this->id_dst.name); + return true; + }; + this->animdata_action_foreach(*this->animdata_dst, actions_idroot_ensure); + } + if (&id_src == &id_dst) { if (this->has_changes) { DEG_id_tag_update(&this->id_src, ID_RECALC_ANIMATION); @@ -479,6 +549,7 @@ class AnimDataConvertor { } return; } + if (this->fcurves_from_src_main_action.is_empty() && this->fcurves_from_src_tmp_action.is_empty() && this->fcurves_from_src_drivers.is_empty()) { @@ -1086,6 +1157,20 @@ static void legacy_gpencil_to_grease_pencil(ConversionData &conversion_data, if (AnimData *gpd_animdata = BKE_animdata_from_id(&gpd.id)) { grease_pencil.adt = BKE_animdata_copy_in_lib( &conversion_data.bmain, gpd.id.lib, gpd_animdata, LIB_ID_COPY_DEFAULT); + + /* Some property was renamed between legacy GP layers and new GreasePencil ones. */ + AnimDataConvertor animdata_gpdata_transfer( + conversion_data, grease_pencil.id, gpd.id, {{".location", ".translation"}}); + for (const Layer *layer_iter : grease_pencil.layers()) { + /* Data comes from versioned GPv2 layers, which have a fixed max length. */ + char layer_name_esc[sizeof((bGPDlayer{}).info) * 2]; + BLI_str_escape(layer_name_esc, layer_iter->name().c_str(), sizeof(layer_name_esc)); + std::string layer_root_path = fmt::format("layers[\"{}\"]", layer_name_esc); + animdata_gpdata_transfer.root_path_dst = layer_root_path; + animdata_gpdata_transfer.root_path_src = layer_root_path; + animdata_gpdata_transfer.fcurves_convert(); + } + animdata_gpdata_transfer.fcurves_convert_finalize(); } }