fix animation reversing

Include/RmlUi/Core/Animation.h

@@ -51,7 +51,6 @@ struct Transition {
 	Tween tween;
 	float duration = 0.0f;
 	float delay = 0.0f;
-	float reverse_adjustment_factor = 0.0f;
 };
 
 struct TransitionList {
@@ -74,8 +73,7 @@ inline bool operator!=(const Animation& a, const Animation& b)
 }
 inline bool operator==(const Transition& a, const Transition& b)
 {
-	return a.id == b.id && a.tween == b.tween && a.duration == b.duration && a.delay == b.delay &&
-		a.reverse_adjustment_factor == b.reverse_adjustment_factor;
+	return a.id == b.id && a.tween == b.tween && a.duration == b.duration && a.delay == b.delay;
 }
 inline bool operator!=(const Transition& a, const Transition& b)
 {

Include/RmlUi/Core/Element.h

@@ -702,7 +702,8 @@ class RMLUICORE_API Element : public ScriptInterface, public EnableObserverPtr<E
 	/// Start a transition of the given property on this element.
 	/// If an animation exists for the property, the call will be ignored. If a transition exists for this property, it will be replaced.
 	/// @return True if the transition was added or replaced.
-	bool StartTransition(const Transition& transition, const Property& start_value, const Property& target_value);
+	bool StartTransition(const Transition& transition, std::vector<ElementAnimation>::iterator& existing_iterator, const Property& start_value,
+		const Property& target_value);
 
 	/// Removes all transitions that are no longer part of the element's 'transition' property.
 	void HandleTransitionProperty();

Source/Core/Element.cpp

@@ -2515,40 +2515,106 @@ bool Element::AddAnimationKeyTime(PropertyId property_id, const Property* target
 	return result;
 }
 
-bool Element::StartTransition(const Transition& transition, const Property& start_value, const Property& target_value)
+bool Element::StartTransition(const Transition& transition, std::vector<ElementAnimation>::iterator& existing_iterator, const Property& start_value,
+	const Property& target_value)
 {
-	auto it = std::find_if(animations.begin(), animations.end(), [&](const ElementAnimation& el) { return el.GetPropertyId() == transition.id; });
+	ElementAnimation* existing_transition = existing_iterator != animations.end() ? &(*existing_iterator) : nullptr;
+	const bool has_running_transition = (existing_transition && !existing_transition->IsComplete());
+	const bool has_completed_transition = (existing_transition && existing_transition->IsComplete());
+	const bool existing_has_different_end_value = (existing_transition && existing_transition->GetEndValue() != &target_value);
 
-	if (it != animations.end() && !it->IsTransition())
-		return false;
+	// start_value and target_value are already checked to be different in the caller
+	// start_value has already been modified by the existing transition (if it exists)
 
-	float duration = transition.duration;
-	double start_time = Clock::GetElapsedTime() + (double)transition.delay;
+	// https://www.w3.org/TR/css-transitions-1/#starting
 
-	if (it == animations.end())
+	// Step 1: standard start from no transition or completed transition
+	if (!has_running_transition && (!has_completed_transition || existing_has_different_end_value) && transition.duration > 0.0f)
 	{
-		// Add transition as new animation
-		animations.push_back(ElementAnimation{transition.id, ElementAnimationOrigin::Transition, start_value, *this, start_time, 0.0f, 1, false});
-		it = (animations.end() - 1);
+		if (existing_transition)
+			animations.erase(existing_iterator);
+
+		double start_time = Clock::GetElapsedTime() + (double)transition.delay;
+		float duration = transition.duration;
+
+		auto new_transition = ElementAnimation::CreateTransition(transition.id, *this, transition.tween, start_time, duration, start_value,
+			target_value, start_value, 1.f);
+
+		if (new_transition.IsValidTransition())
+		{
+			animations.push_back(std::move(new_transition));
+			SetProperty(transition.id, start_value);
+			return true;
+		}
 	}
-	else
+	// Step 2: remove completed transition if it has different end value, do not start new transition
+	else if (has_completed_transition && existing_has_different_end_value)
 	{
-		// Compress the duration based on the progress of the current animation
-		float f = it->GetInterpolationFactor();
-		f = 1.0f - (1.0f - f) * transition.reverse_adjustment_factor;
-		duration = duration * f;
-		// Replace old transition
-		*it = ElementAnimation{transition.id, ElementAnimationOrigin::Transition, start_value, *this, start_time, 0.0f, 1, false};
+		animations.erase(existing_iterator);
 	}
+	// Step 3: transition was removed from the element's style
+	// this is taken care of in Element::HandleTransitionProperty()
+	// Step 4: replace running transition
+	else if (has_running_transition && existing_has_different_end_value)
+	{
+		// Step 4.1: cancel running transition if new transition would do nothing
+		// taken care of in Element::StartTransition just before calling this function
+		// Step 4.2: cancel running transition if new transition would be invalid
+		// supposed to also check if the property is "transitionable", but that is hard to determine
+		if (transition.duration <= 0.0f)
+		{
+			animations.erase(existing_iterator);
+		}
+		// Step 4.3: replace running transition with special reversing transition
+		else if (existing_transition && existing_transition->GetReversingAdjustedStartValue() == &target_value)
+		{
+			float reversing_shortening_factor = existing_transition->GetReversingShorteningFactor();
+			float new_reversing_shortening_factor = std::abs(existing_transition->GetInterpolationFactor() * reversing_shortening_factor);
+			new_reversing_shortening_factor = Math::Clamp(new_reversing_shortening_factor, 0.f, 1.f);
 
-	bool result = it->AddKey(duration, target_value, *this, transition.tween, true);
+			double start_time = Clock::GetElapsedTime() + (double)transition.delay;
+			start_time = (transition.delay >= 0.0f ? start_time : start_time * new_reversing_shortening_factor);
+			float new_duration = transition.duration * new_reversing_shortening_factor;
 
-	if (result)
-		SetProperty(transition.id, start_value);
-	else
-		animations.erase(it);
+			auto new_transition = ElementAnimation::CreateTransition(transition.id, *this, transition.tween, start_time, new_duration, start_value,
+				target_value, *(existing_transition->GetEndValue()), new_reversing_shortening_factor);
 
-	return result;
+			if (new_transition.IsValidTransition())
+			{
+				// replace existing transition in place
+				*existing_iterator = std::move(new_transition);
+				SetProperty(transition.id, start_value);
+				return true;
+			}
+			else
+			{
+				animations.erase(existing_iterator);
+			}
+		}
+		// Step 4.4: replace running transition with entirely new transition
+		else
+		{
+			double start_time = Clock::GetElapsedTime() + (double)transition.delay;
+			float duration = transition.duration;
+
+			auto new_transition = ElementAnimation::CreateTransition(transition.id, *this, transition.tween, start_time, duration, start_value,
+				target_value, start_value, 1.f);
+
+			if (new_transition.IsValidTransition())
+			{
+				// replace existing transition in place
+				*existing_iterator = std::move(new_transition);
+				SetProperty(transition.id, start_value);
+				return true;
+			}
+			else
+			{
+				animations.erase(existing_iterator);
+			}
+		}
+	}
+
+	return false;
 }
 
 void Element::HandleTransitionProperty()

Source/Core/ElementAnimation.cpp

@@ -756,4 +756,14 @@ Property ElementAnimation::UpdateAndGetProperty(double world_time, Element& elem
 	return result;
 }
 
+ElementAnimation ElementAnimation::CreateTransition(PropertyId property_id, Element& element, Tween tween, double start_time, float duration,
+	const Property& start_value, const Property& end_value, const Property& reversing_adjusted_start_value, float reversing_shortening_factor)
+{
+	auto new_transition = ElementAnimation(property_id, ElementAnimationOrigin::Transition, start_value, element, start_time, duration, 1, false);
+	new_transition.InternalAddKey(duration, end_value, element, tween);
+	new_transition.reversing_adjusted_start_value = Rml::MakeUnique<Property>(reversing_adjusted_start_value);
+	new_transition.reversing_shortening_factor = reversing_shortening_factor;
+	return new_transition;
+}
+
 } // namespace Rml

Source/Core/ElementAnimation.h

@@ -66,6 +66,10 @@ class ElementAnimation {
 	bool animation_complete = false;
 	ElementAnimationOrigin origin = ElementAnimationOrigin::User;
 
+	// transition-only fields
+	Rml::UniquePtr<Property> reversing_adjusted_start_value;
+	float reversing_shortening_factor = 1.0f;
+
 	bool InternalAddKey(float time, const Property& property, Element& element, Tween tween);
 
 	float GetInterpolationFactorAndKeys(int* out_key0, int* out_key1) const;
@@ -86,6 +90,18 @@ class ElementAnimation {
 	bool IsInitalized() const { return !keys.empty(); }
 	float GetInterpolationFactor() const { return GetInterpolationFactorAndKeys(nullptr, nullptr); }
 	ElementAnimationOrigin GetOrigin() const { return origin; }
+
+	const Property* GetStartValue() { return &(keys[0].property); }
+	const Property* GetEndValue() { return &(keys[keys.size() - 1].property); }
+
+	// Transition-related getters
+	const Property* GetReversingAdjustedStartValue() const { return reversing_adjusted_start_value.get(); }
+	float GetReversingShorteningFactor() const { return reversing_shortening_factor; }
+	bool IsValidTransition() const { return IsTransition() && keys.size() == 2; }
+
+	// arguments based on: https://www.w3.org/TR/css-transitions-1/#ref-for-completed-transition%E2%91%A1
+	static ElementAnimation CreateTransition(PropertyId property_id, Element& element, Tween tween, double start_time, float duration,
+		const Property& start_value, const Property& end_value, const Property& reversing_adjusted_start_value, float reversing_shortening_factor);
 };
 
 } // namespace Rml

Source/Core/ElementStyle.cpp

@@ -44,6 +44,7 @@
 #include "../../Include/RmlUi/Core/StyleSheetSpecification.h"
 #include "../../Include/RmlUi/Core/TransformPrimitive.h"
 #include "ComputeProperty.h"
+#include "ElementAnimation.h"
 #include "ElementDefinition.h"
 #include "PropertiesIterator.h"
 #include <algorithm>
@@ -108,59 +109,69 @@ const Property* ElementStyle::GetProperty(PropertyId id, const Element* element,
 	return property->GetDefaultValue();
 }
 
-void ElementStyle::TransitionPropertyChanges(Element* element, PropertyIdSet& properties, const PropertyDictionary& inline_properties,
-	const ElementDefinition* old_definition, const ElementDefinition* new_definition)
+void ElementStyle::ApplyTransitionDefinitionChanges(PropertyIdSet& changed_properties, const ElementDefinition* old_definition,
+	const ElementDefinition* new_definition)
 {
-	// Apply transition to relevant properties if a transition is defined on element.
-	// Properties that are part of a transition are removed from the properties list.
+	RMLUI_ZoneScoped;
+
+	if (!old_definition || !new_definition || changed_properties.Empty())
+		return;
 
-	RMLUI_ASSERT(element);
-	if (!old_definition || !new_definition || properties.Empty())
+	const Property* new_transition_property = GetLocalProperty(PropertyId::Transition, inline_properties, new_definition);
+	if (!new_transition_property || new_transition_property->value.GetType() != Variant::TRANSITIONLIST)
 		return;
 
-	// We get the local property instead of the computed value here, because we want to intercept property changes even before the computed values are
-	// ready. Now that we have the concept of computed values, we may want do this operation directly on them instead.
-	if (const Property* transition_property = GetLocalProperty(PropertyId::Transition, inline_properties, new_definition))
+	const auto& transition_list = new_transition_property->value.GetReference<TransitionList>();
+
+	if (!transition_list.none)
 	{
-		if (transition_property->value.GetType() != Variant::TRANSITIONLIST)
-			return;
+		static const PropertyDictionary empty_properties;
+
+		auto add_transition = [&](const Transition& transition) {
+			auto existing_iterator = std::find_if(element->animations.begin(), element->animations.end(),
+				[&](const ElementAnimation& el) { return el.GetPropertyId() == transition.id; });
 
-		const TransitionList& transition_list = transition_property->value.GetReference<TransitionList>();
+			if (existing_iterator != element->animations.end() && !existing_iterator->IsTransition())
+				return false;
+
+			bool transition_added = false;
+			const Property* start_value = GetProperty(transition.id, element, inline_properties, old_definition);
+			const Property* target_value = GetProperty(transition.id, element, empty_properties, new_definition);
+			if (start_value && target_value && (*start_value != *target_value))
+			{
+				transition_added = element->StartTransition(transition, existing_iterator, *start_value, *target_value);
+			}
+			else if (existing_iterator != element->animations.end() && !existing_iterator->IsComplete())
+			{
+				// https://www.w3.org/TR/css-transitions-1/#starting
+				// Step 4.1: cancel running transition if new transition would do nothing
+				// ... or additionally, if the values can't be determined
+				element->animations.erase(existing_iterator);
+			}
 
-		if (!transition_list.none)
+			return transition_added;
+		};
+
+		if (transition_list.all)
 		{
-			static const PropertyDictionary empty_properties;
-
-			auto add_transition = [&](const Transition& transition) {
-				bool transition_added = false;
-				const Property* start_value = GetProperty(transition.id, element, inline_properties, old_definition);
-				const Property* target_value = GetProperty(transition.id, element, empty_properties, new_definition);
-				if (start_value && target_value && (*start_value != *target_value))
-					transition_added = element->StartTransition(transition, *start_value, *target_value);
-				return transition_added;
-			};
-
-			if (transition_list.all)
+			Transition transition = transition_list.transitions[0];
+			for (auto it = changed_properties.begin(); it != changed_properties.end();)
 			{
-				Transition transition = transition_list.transitions[0];
-				for (auto it = properties.begin(); it != properties.end();)
-				{
-					transition.id = *it;
-					if (add_transition(transition))
-						it = properties.Erase(it);
-					else
-						++it;
-				}
+				transition.id = *it;
+				if (add_transition(transition))
+					it = changed_properties.Erase(it);
+				else
+					++it;
 			}
-			else
+		}
+		else
+		{
+			for (const Transition& transition : transition_list.transitions)
 			{
-				for (const Transition& transition : transition_list.transitions)
+				if (changed_properties.Contains(transition.id))
 				{
-					if (properties.Contains(transition.id))
-					{
-						if (add_transition(transition))
-							properties.Erase(transition.id);
-					}
+					if (add_transition(transition))
+						changed_properties.Erase(transition.id);
 				}
 			}
 		}
@@ -203,7 +214,7 @@ void ElementStyle::UpdateDefinition()
 			}
 
 			// Transition changed properties if transition property is set
-			TransitionPropertyChanges(element, changed_properties, inline_properties, definition.get(), new_definition.get());
+			ApplyTransitionDefinitionChanges(changed_properties, definition.get(), new_definition.get());
 		}
 
 		definition = new_definition;

Source/Core/ElementStyle.h

@@ -150,8 +150,16 @@ class ElementStyle {
 	static const Property* GetLocalProperty(PropertyId id, const PropertyDictionary& inline_properties, const ElementDefinition* definition);
 	static const Property* GetProperty(PropertyId id, const Element* element, const PropertyDictionary& inline_properties,
 		const ElementDefinition* definition);
-	static void TransitionPropertyChanges(Element* element, PropertyIdSet& properties, const PropertyDictionary& inline_properties,
-		const ElementDefinition* old_definition, const ElementDefinition* new_definition);
+
+	/** Called from Element::UpdateDefinition() just before dirty_properties is set.
+	 * https://www.w3.org/TR/css-transitions-1/
+	 * @param[in, out] changed_properties Set of changed properties ids. Properties that will be handled by a newly created transition are removed
+	 * from this set.
+	 * @param[in] old_definition
+	 * @param[in] new_definition
+	 */
+	void ApplyTransitionDefinitionChanges(PropertyIdSet& changed_properties, const ElementDefinition* old_definition,
+		const ElementDefinition* new_definition);
 
 	// Element these properties belong to
 	Element* element;

Source/Core/PropertyParserAnimation.cpp

@@ -225,7 +225,6 @@ static bool ParseTransition(Property& property, const StringList& transition_val
 
 		bool duration_found = false;
 		bool delay_found = false;
-		bool reverse_adjustment_factor_found = false;
 
 		for (auto& argument : arguments)
 		{
@@ -279,17 +278,6 @@ static bool ParseTransition(Property& property, const StringList& transition_val
 						else
 							return false;
 					}
-					else
-					{
-						// No 's' unit means reverse adjustment factor was found
-						if (!reverse_adjustment_factor_found)
-						{
-							reverse_adjustment_factor_found = true;
-							transition.reverse_adjustment_factor = number;
-						}
-						else
-							return false;
-					}
 				}
 				else
 				{
@@ -317,8 +305,6 @@ static bool ParseTransition(Property& property, const StringList& transition_val
 		if ((transition_list.all && !target_property_ids.Empty())    //
 			|| (!transition_list.all && target_property_ids.Empty()) //
 			|| transition.duration <= 0.0f                           //
-			|| transition.reverse_adjustment_factor < 0.0f           //
-			|| transition.reverse_adjustment_factor > 1.0f           //
 		)
 		{
 			return false;

Source/Core/TypeConverter.cpp

@@ -123,8 +123,6 @@ bool TypeConverter<TransitionList, String>::Convert(const TransitionList& src, S
 			dest += tmp + "s ";
 		if (t.delay > 0.0f && TypeConverter<float, String>::Convert(t.delay, tmp))
 			dest += tmp + "s ";
-		if (t.reverse_adjustment_factor > 0.0f && TypeConverter<float, String>::Convert(t.reverse_adjustment_factor, tmp))
-			dest += tmp + ' ';
 		if (dest.size() > 0)
 			dest.resize(dest.size() - 1);
 		if (i != src.transitions.size() - 1)