Merge pull request #117030 from ugursoy/rehash-on-creation

Use IDs instead of hashes for TrackCache keys

scene/animation/animation_mixer.cpp

@@ -160,7 +160,6 @@ void AnimationMixer::_animation_set_cache_update() {
 	for (const AnimationLibraryData &lib : animation_libraries) {
 		for (const KeyValue<StringName, Ref<Animation>> &K : lib.library->animations) {
 			StringName key = lib.name == StringName() ? K.key : StringName(String(lib.name) + "/" + String(K.key));
-
 			AnimationData *ad = animation_set.getptr(key);
 
 			if (!ad) {
@@ -588,7 +587,7 @@ void AnimationMixer::_clear_caches() {
 	_init_root_motion_cache();
 	_clear_audio_streams();
 	_clear_playing_caches();
-	for (KeyValue<Animation::TypeHash, TrackCache *> &K : track_cache) {
+	for (KeyValue<Animation::TrackCacheID, TrackCache *> &K : track_cache) {
 		memdelete(K.value);
 	}
 	track_cache.clear();
@@ -640,7 +639,7 @@ void AnimationMixer::_create_track_num_to_track_cache_for_animation(const Ref<An
 
 	track_num_to_track_cache.resize(tracks.size());
 	for (uint32_t i = 0; i < tracks.size(); i++) {
-		TrackCache **track_ptr = track_cache.getptr(tracks[i]->thash);
+		TrackCache **track_ptr = track_cache.getptr(tracks[i]->get_unique_id());
 		if (track_ptr == nullptr) {
 			track_num_to_track_cache[i] = nullptr;
 		} else {
@@ -699,12 +698,12 @@ bool AnimationMixer::_update_caches() {
 			NodePath path = anim->track_get_path(i);
 			(void)path.hash(); // Make sure the cache is valid for faster comparison.
 
-			Animation::TypeHash thash = anim->track_get_type_hash(i);
+			const Animation::TrackCacheID &track_unique_id = anim->track_get_unique_id(i);
 			Animation::TrackType track_src_type = anim->track_get_type(i);
 			Animation::TrackType track_cache_type = Animation::get_cache_type(track_src_type);
 
 			TrackCache *track = nullptr;
-			if (TrackCache **p = track_cache.getptr(thash)) {
+			if (TrackCache **p = track_cache.getptr(track_unique_id)) {
 				track = *p;
 			}
 
@@ -712,7 +711,7 @@ bool AnimationMixer::_update_caches() {
 			if (track && (track->type != track_cache_type || ObjectDB::get_instance(track->object_id) == nullptr)) {
 				playing_caches.erase(track);
 				memdelete(track);
-				track_cache.erase(thash);
+				track_cache.erase(track_unique_id);
 				track = nullptr;
 			}
 
@@ -923,7 +922,7 @@ bool AnimationMixer::_update_caches() {
 					}
 				}
 				track->path = path;
-				track_cache[thash] = track;
+				track_cache[track_unique_id] = track;
 			} else if (track_cache_type == Animation::TYPE_POSITION_3D) {
 				TrackCacheTransform *track_xform = static_cast<TrackCacheTransform *>(track);
 				if (track->setup_pass != setup_pass) {
@@ -963,28 +962,28 @@ bool AnimationMixer::_update_caches() {
 		}
 	}
 
-	LocalVector<Animation::TypeHash> to_delete;
+	LocalVector<Animation::TrackCacheID> to_delete;
 
-	for (const KeyValue<Animation::TypeHash, TrackCache *> &K : track_cache) {
+	for (const KeyValue<Animation::TrackCacheID, TrackCache *> &K : track_cache) {
 		if (K.value->setup_pass != setup_pass) {
 			to_delete.push_back(K.key);
 		}
 	}
 
-	for (const Animation::TypeHash &thash : to_delete) {
-		memdelete(track_cache[thash]);
-		track_cache.erase(thash);
+	for (const Animation::TrackCacheID &unique_id : to_delete) {
+		memdelete(track_cache[unique_id]);
+		track_cache.erase(unique_id);
 	}
 
 	track_map.clear();
 
 	int idx = 0;
-	for (const KeyValue<Animation::TypeHash, TrackCache *> &K : track_cache) {
+	for (const KeyValue<Animation::TrackCacheID, TrackCache *> &K : track_cache) {
 		track_map[K.value->path] = idx;
 		idx++;
 	}
 
-	for (KeyValue<Animation::TypeHash, TrackCache *> &K : track_cache) {
+	for (KeyValue<Animation::TrackCacheID, TrackCache *> &K : track_cache) {
 		K.value->blend_idx = track_map[K.value->path];
 	}
 
@@ -1066,7 +1065,7 @@ void AnimationMixer::_blend_init() {
 	}
 
 	// Init all value/transform/blend/bezier tracks that track_cache has.
-	for (const KeyValue<Animation::TypeHash, TrackCache *> &K : track_cache) {
+	for (const KeyValue<Animation::TrackCacheID, TrackCache *> &K : track_cache) {
 		TrackCache *track = K.value;
 
 		track->total_weight = 0.0;
@@ -1171,7 +1170,7 @@ void AnimationMixer::_blend_calc_total_weight() {
 		uint64_t pass_id = ++animation_instance_weight_pass_counter;
 		// Handle wrap (slower but rare).
 		if (unlikely(pass_id == 0)) {
-			for (KeyValue<Animation::TypeHash, TrackCache *> &kv : track_cache) {
+			for (KeyValue<Animation::TrackCacheID, TrackCache *> &kv : track_cache) {
 				if (kv.value) {
 					kv.value->animation_instance_weight_applied_at = 0;
 				}
@@ -1897,7 +1896,7 @@ void AnimationMixer::_blend_process(double p_delta, bool p_update_only) {
 
 void AnimationMixer::_blend_apply() {
 	// Finally, set the tracks.
-	for (const KeyValue<Animation::TypeHash, TrackCache *> &K : track_cache) {
+	for (const KeyValue<Animation::TrackCacheID, TrackCache *> &K : track_cache) {
 		TrackCache *track = K.value;
 		bool is_zero_amount = Math::is_zero_approx(track->total_weight);
 		if (!deterministic && is_zero_amount) {
@@ -2170,7 +2169,7 @@ bool AnimationMixer::can_apply_reset() const {
 }
 
 void AnimationMixer::_build_backup_track_cache() {
-	for (const KeyValue<Animation::TypeHash, TrackCache *> &K : track_cache) {
+	for (const KeyValue<Animation::TrackCacheID, TrackCache *> &K : track_cache) {
 		TrackCache *track = K.value;
 		track->total_weight = 1.0;
 		switch (track->type) {
@@ -2267,7 +2266,7 @@ Ref<AnimatedValuesBackup> AnimationMixer::make_backup() {
 	make_animation_instance(SceneStringName(RESET), pi);
 	_build_backup_track_cache();
 
-	backup->set_data(AHashMap<Animation::TypeHash, TrackCache *, HashHasher>(track_cache));
+	backup->set_data(AHashMap<Animation::TrackCacheID, TrackCache *, HashHasher>(track_cache));
 	clear_animation_instances();
 
 	return backup;
@@ -2299,7 +2298,7 @@ void AnimationMixer::restore(const Ref<AnimatedValuesBackup> &p_backup) {
 	ERR_FAIL_COND(p_backup.is_null());
 	track_cache = p_backup->get_data();
 	_blend_apply();
-	track_cache = AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher>();
+	track_cache = AHashMap<Animation::TrackCacheID, AnimationMixer::TrackCache *, HashHasher>();
 	cache_valid = false;
 }
 
@@ -2357,7 +2356,7 @@ void AnimationMixer::capture(const StringName &p_name, double p_duration, Tween:
 			continue;
 		}
 		if (reference_animation->track_get_type(i) == Animation::TYPE_VALUE && reference_animation->value_track_get_update_mode(i) == Animation::UPDATE_CAPTURE) {
-			TrackCacheValue *t = static_cast<TrackCacheValue *>(track_cache[reference_animation->track_get_type_hash(i)]);
+			TrackCacheValue *t = static_cast<TrackCacheValue *>(track_cache[reference_animation->track_get_unique_id(i)]);
 			Object *t_obj = ObjectDB::get_instance(t->object_id);
 			if (t_obj) {
 				Variant value = t_obj->get_indexed(t->subpath);
@@ -2541,10 +2540,10 @@ AnimationMixer::AnimationMixer() {
 AnimationMixer::~AnimationMixer() {
 }
 
-void AnimatedValuesBackup::set_data(const AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher> &p_data) {
+void AnimatedValuesBackup::set_data(const AHashMap<Animation::TrackCacheID, AnimationMixer::TrackCache *, HashHasher> &p_data) {
 	clear_data();
 
-	for (const KeyValue<Animation::TypeHash, AnimationMixer::TrackCache *> &E : p_data) {
+	for (const KeyValue<Animation::TrackCacheID, AnimationMixer::TrackCache *> &E : p_data) {
 		AnimationMixer::TrackCache *track = get_cache_copy(E.value);
 		if (!track) {
 			continue; // Some types of tracks do not get a copy and must be ignored.
@@ -2554,9 +2553,9 @@ void AnimatedValuesBackup::set_data(const AHashMap<Animation::TypeHash, Animatio
 	}
 }
 
-AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher> AnimatedValuesBackup::get_data() const {
-	AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher> ret;
-	for (const KeyValue<Animation::TypeHash, AnimationMixer::TrackCache *> &E : data) {
+AHashMap<Animation::TrackCacheID, AnimationMixer::TrackCache *, HashHasher> AnimatedValuesBackup::get_data() const {
+	AHashMap<Animation::TrackCacheID, AnimationMixer::TrackCache *, HashHasher> ret;
+	for (const KeyValue<Animation::TrackCacheID, AnimationMixer::TrackCache *> &E : data) {
 		AnimationMixer::TrackCache *track = get_cache_copy(E.value);
 		ERR_CONTINUE(!track); // Backup shouldn't contain tracks that cannot be copied, this is a mistake.
 
@@ -2566,7 +2565,7 @@ AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher> Animated
 }
 
 void AnimatedValuesBackup::clear_data() {
-	for (KeyValue<Animation::TypeHash, AnimationMixer::TrackCache *> &K : data) {
+	for (KeyValue<Animation::TrackCacheID, AnimationMixer::TrackCache *> &K : data) {
 		memdelete(K.value);
 	}
 	data.clear();

scene/animation/animation_mixer.h

@@ -308,7 +308,7 @@ protected:
 	};
 
 	RootMotionCache root_motion_cache;
-	AHashMap<Animation::TypeHash, TrackCache *, HashHasher> track_cache;
+	AHashMap<Animation::TrackCacheID, TrackCache *, HashHasher> track_cache;
 	AHashMap<Ref<Animation>, LocalVector<TrackCache *>> animation_track_num_to_track_cache;
 	HashSet<TrackCache *> playing_caches;
 	Vector<Node *> playing_audio_stream_players;
@@ -492,11 +492,12 @@ public:
 class AnimatedValuesBackup : public RefCounted {
 	GDCLASS(AnimatedValuesBackup, RefCounted);
 
-	AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher> data;
+	AHashMap<Animation::TrackCacheID, AnimationMixer::TrackCache *, HashHasher> data;
 
 public:
-	void set_data(const AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher> &p_data);
-	AHashMap<Animation::TypeHash, AnimationMixer::TrackCache *, HashHasher> get_data() const;
+	void set_data(const AHashMap<Animation::TrackCacheID, AnimationMixer::TrackCache *, HashHasher> &p_data);
+	AHashMap<Animation::TrackCacheID, AnimationMixer::TrackCache *, HashHasher> get_data() const;
+
 	void clear_data();
 
 	AnimationMixer::TrackCache *get_cache_copy(AnimationMixer::TrackCache *p_cache) const;

scene/resources/animation.cpp

@@ -1034,7 +1034,7 @@ Animation::TrackType Animation::track_get_type(int p_track) const {
 void Animation::track_set_path(int p_track, const NodePath &p_path) {
 	ERR_FAIL_UNSIGNED_INDEX((uint32_t)p_track, tracks.size());
 	tracks[p_track]->path = p_path;
-	_track_update_hash(p_track);
+	tracks[p_track]->concatenated_path = StringName(String(tracks[p_track]->path));
 	emit_changed();
 }
 
@@ -1062,15 +1062,9 @@ Animation::TrackType Animation::get_cache_type(TrackType p_type) {
 	return p_type;
 }
 
-void Animation::_track_update_hash(int p_track) {
-	const NodePath &track_path = tracks[p_track]->path;
-	const TrackType track_cache_type = get_cache_type(tracks[p_track]->type);
-	tracks[p_track]->thash = HashMapHasherDefault::hash(Pair<const NodePath &, TrackType>(track_path, track_cache_type));
-}
-
-Animation::TypeHash Animation::track_get_type_hash(int p_track) const {
+Animation::TrackCacheID Animation::track_get_unique_id(int p_track) const {
 	ERR_FAIL_UNSIGNED_INDEX_V((uint32_t)p_track, tracks.size(), 0);
-	return tracks[p_track]->thash;
+	return tracks[p_track]->get_unique_id();
 }
 
 void Animation::track_set_interpolation_type(int p_track, InterpolationType p_interp) {

scene/resources/animation.h

@@ -40,7 +40,7 @@ class Animation : public Resource {
 	RES_BASE_EXTENSION("anim");
 
 public:
-	typedef uint32_t TypeHash;
+	typedef uint64_t TrackCacheID;
 
 	static inline String PARAMETERS_BASE_PATH = "parameters/";
 	static constexpr real_t DEFAULT_STEP = 1.0 / 30;
@@ -109,9 +109,16 @@ public:
 		InterpolationType interpolation = INTERPOLATION_LINEAR;
 		bool loop_wrap = true;
 		NodePath path; // Path to something.
-		TypeHash thash = 0; // Hash by Path + SubPath + TrackType.
+		StringName concatenated_path;
 		bool imported = false;
 		bool enabled = true;
+		TrackCacheID get_unique_id() const {
+			// HACK. StringName pointer allocates 40 bytes (sizeof(StringName::_Data)), so any StringName pointer we allocate
+			// is guaranteed to not have another StringName pointer within 40 bytes address range. get_cache_type() returns
+			// a value in range (0, 8) so this can be used as an offset while keeping the uniqueness property.
+			// This will break if sizeof(StringName::_Data) is less than max(CACHE_TYPE).
+			return (TrackCacheID)(concatenated_path.data_unique_pointer()) + get_cache_type(type);
+		}
 		virtual ~Track() {}
 	};
 
@@ -250,7 +257,6 @@ private:
 	HashMap<StringName, Color> marker_colors; // name -> color
 
 	LocalVector<Track *> tracks;
-
 #ifdef TOOLS_ENABLED
 	HashSet<StringName> folded_groups;
 #endif // TOOLS_ENABLED
@@ -288,8 +294,6 @@ private:
 	bool capture_included = false;
 	void _check_capture_included();
 
-	void _track_update_hash(int p_track);
-
 	/* Animation compression page format (version 1):
 	 *
 	 * Animation uses bitwidth based compression separated into small pages. The intention is that pages fit easily in the cache, so decoding is cache efficient.
@@ -424,7 +428,7 @@ public:
 	NodePath track_get_path(int p_track) const;
 	int find_track(const NodePath &p_path, const TrackType p_type) const;
 
-	TypeHash track_get_type_hash(int p_track) const;
+	TrackCacheID track_get_unique_id(int p_track) const;
 
 	void track_move_up(int p_track);
 	void track_move_down(int p_track);