Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Fix freezing after inactivity #1776

Merged
merged 13 commits into from
Sep 29, 2021
Merged

Fix freezing after inactivity #1776

merged 13 commits into from
Sep 29, 2021

Conversation

s9ferech
Copy link
Contributor

@s9ferech s9ferech commented Aug 10, 2021

Pull Request Description

This PR fixes issue #1748.

It solves multiple problems:

  1. Animation state is handled relative to the target value. For animations of floats or similar types, this means that the numbers get smaller as the animation approaches its target value. This leads to higher precision during computations and avoids a bug where the termination condition was never satisfied due to rounding errors with large numbers.
  2. We can properly handle animations where the distance between the start and target value is NaN. Those will just instantly skip to the target value.
  3. We initialize the graph editors collection of profiling statuses with the correct minimum and maximum.
  4. Profiling labels will have an actual color, even when there is no profiling information and the label is invisible. Before, they they assumed a color with hue NaN in that situation. But this might lead to problems, since the color types were likely not intended to be used with such values.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The CHANGELOG.md was updated with the changes introduced in this PR.
  • The documentation has been updated if necessary.
  • All code conforms to the Rust style guide.
  • All code has automatic tests where possible.
  • All code has been profiled where possible.
  • All code has been manually tested in the IDE.
  • All code has been manually tested in the "debug/interface" scene.
  • All code has been manually tested by the PR owner against our test scenarios.
  • All code has been manually tested by at least one reviewer against our test scenarios.

@s9ferech s9ferech added Category: BaseGL The BaseGL library Type: Bug A bug in Enso IDE Category: GUI The Graphical User Interface labels Aug 10, 2021
@s9ferech s9ferech self-assigned this Aug 10, 2021
@s9ferech s9ferech requested a review from wdanilo as a code owner August 10, 2021 12:58
Comment on lines 223 to 227
// We store the current value as an offset from the target rather than an absolute value,
// because this provides higher precision when the value is getting close to the target and `T`
// is a type of floating-point numbers, vectors of floating point numbers, or similar. The main
// benefit is that we avoid non-termination that could otherwise happen due to rounding errors
// when the target and current value are large.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this description. "this provides higher precision when the value is getting close to the target" - why? Could you describe it with some examples and better explanation, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to improve the explanation. Let me know if it is better.

Comment on lines 736 to 752
fn animation_to_nan() {
let mut data = SimulationData::<f32>::new();
data.set_value(0.0);
data.set_target_value(f32::NAN);
data.step(1.0);
assert!(data.value().is_nan());
assert!(!data.active);
}

#[test]
fn animation_from_nan() {
let mut data = SimulationData::<f32>::new();
data.set_value(f32::NAN);
data.set_target_value(0.0);
data.step(1.0);
assert_eq!(data.value(),0.0);
assert!(!data.active);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add comments what these tests are for and how they should behave. They are testing a corner cases and this needs additional explanation

@s9ferech s9ferech requested a review from wdanilo August 17, 2021 12:07
@s9ferech
Copy link
Contributor Author

As mentioned here, I propose to remove the warning for large list views as part of this PR. What do you think @wdanilo?

CHANGELOG.md Outdated
editing node expression, the changes were occasionally reverted, or the
grayed-out parameter names were added to the actual expression.
- [Fixed freezing after inactivity.][1776] When the IDE window was minimized or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the "Next Release" section.

self.velocity = default();
self.active = false;
if should_snap || distance.is_nan() {
self.offset_from_target = default();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a ::zero() method available for Value? That might be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we do. But I agree with you. Maybe the Zero trait should be included in the definition of Value. But the code used default() for zero before. It might have been a conscious decision.

let spring_stretch = value_delta.magnitude();
let coefficient = spring_stretch * self.spring.value;
value_delta.normalize() * coefficient
self.offset_from_target * -self.spring.value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this subtraction is written as -value, while other ones are value * -1.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first one is more readable. But the second one is necessary for values satisfying only the trait Value, because we do not have subtraction on that trait.

@MichaelMauderer MichaelMauderer merged commit c67538b into develop Sep 29, 2021
mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Category: BaseGL The BaseGL library Category: GUI The Graphical User Interface Type: Bug A bug in Enso IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants